tP comments dashboard

Sorted based on the number of comments given to others' PRs, but also showing comments on own PRs and other comments given.

[This page was last updated on Oct 30 2020]

Who dis

I think it would be better to have


if (!csvFile.isFile()) {

    // handle error

    return null;

}

so that we don't have all the main logic inside an if-block.

Over here output is instantiated in every iteration of the loop. Is this intended?

Is it supposed to be split(",") instead?

HAHA nice

The To at the end of the method name feels unnecessary. Shall we use changeAttributes? Or editAttributes or edit which I think will match the user functionality more closely too.

I think naming this method as addTask would be better as it is consistent with the other method names such as editTask and deleteTask.

I think getTaskDescription would be a more descriptive name for this method?

Consider renaming s and d to more descriptive names? Like cmd and details

More descriptive names for t, u, and s?

Why are these fields static? I also think they can just be private instead of protected.

Why static?

Same as above, consider private instead of protected static?

Why you got a space after the ui. lol

Same as above, consider private instead of protected static?

We should refactor this part out since it is repeated like 10 times, but I think it is ok to do it after tomorrow's meeting.

Moving lines 84,85 to right before line 116 here should fix the checkstyle error


Error: eckstyle] [ERROR] /home/runner/work/tp/tp/src/main/java/athena/Parser.java:84: Distance between variable 'name' declaration and its first usage is 6, but allowed 3.  Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value). [VariableDeclarationUsageDistance]

Error: eckstyle] [ERROR] /home/runner/work/tp/tp/src/main/java/athena/Parser.java:85: Distance between variable 'time' declaration and its first usage is 6, but allowed 3.  Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value). [VariableDeclarationUsageDistance]

These 2 methods look like they are doing almost the same thing. Can we combine them together?

Need an empty line before method definition.


Error: eckstyle] [ERROR] /home/runner/work/tp/tp/src/main/java/athena/ListCommand.java:17: 'METHOD_DEF' should be separated from previous statement. [EmptyLineSeparator]

Need a space before taskDuration.

Prof said cannot using * to import. Refer to https://github.com/nus-cs2113-AY2021S1/forum/issues/36


Error: eckstyle] [ERROR] /home/runner/work/tp/tp/src/main/java/athena/Ui.java:37: Line is longer than 120 characters (found 132). [LineLength]


Error: eckstyle] [ERROR] /home/runner/work/tp/tp/src/main/java/athena/Ui.java:39:91: WhitespaceAround: '+' is not followed by whitespace. Empty blocks may only be represented as {} when not part of a multi-block statement (4.1.3) [WhitespaceAround]


Error: eckstyle] [ERROR] /home/runner/work/tp/tp/src/main/java/athena/Ui.java:63: First sentence of Javadoc is missing an ending period. [SummaryJavadoc]


Error: eckstyle] [ERROR] /home/runner/work/tp/tp/src/main/java/athena/Ui.java:75: Line is longer than 120 characters (found 127). [LineLength]

We should put these things in our JUnit tests. I think it's ok if you want to do it in a different pull request.

Code here is the same as lines 15-17. We can do this(taskList) to avoid repetition.

Space between =.

Just a nitpick: counter++ can ignore if you don't care lol

Repeated code. Can do addTask(name, startTime, duration, deadline, recurrence, importance, notes, index).

Would be nice to make this a test case.

Wouldn't the index that we assign be different from its index in the list? tasks.remove(task) instead?

Adding this parameter will cause the program to not compile. We can leave out this parameter for now.

execute has void return-type, doesn't return anything
Should be deleteCommand instead of DeleteCommand

ui needs to be instantiated otherwise it will crash when execute tries to run it.

Currently we don't return the printed message as a string so I think we can leave this line out for now. We should later think about how we want to return the printed string for testing purposes.

Both tasks here are assigned the same index.

deleteCommand.execute will call ui.printTaskDeleted, but ui is null so it will crash.

taskFilter should now be importanceFilter

Continuing the comment in TaskList about getFilteredList(TaskFilter taskFilter), we can do getFilteredList(importanceFilter).getFilteredList(forecastFilter).

We can just do return true here. But I think this is fine too.

I think we don't need to add another ForecastFilter parameter because ForecastFilter is a subclass of TaskFilter. In the Timetable class I elaborated on how this can be used.

With this, it will be possible to use any type of filters as we add more in the future (for example minus importance filter).

Was this for your own testing?

I think it is more sustainable to generate a date for this test case.

We should generate a date for this test case.

lol got this here

We should remove these commented code since we are not using it anymore. If we wanna refer back we can find the old version here on GitHub.

We shouldn't instantiate the Scanner in every iteration of the loop. From experience this causes problems with the IO redirection tests.

taskList would be a better name than oldTask, I believe?

reccurence spelt wrongly lol

I think we can change Objects.equals(taskForecast, that.taskForecast) to taskForecast == that.taskForecast since we use the enum now instead of a String.

Yea I think something like getTask would be nicer. Also there should be a JavaDoc comment.

Lowercase L in List?

will still execute in a default way.

How about will execute using the default values for each parameter?

the name of the task instead of Name of the task? And same for the other parameters.

printed with an index

How about printed with a number because index sounds a bit like jargon?

the name of the task instead of Name? And same for the other parameters.

Unnecessary empty lines

Unnecessary empty line

Empty lines

It would be better override equals in TaskList instead

https://github.com/AY2021S1-CS2113T-W12-2/tp/blob/30bf904954319f483b762ee3c7f5a378e494c4b7/src/main/java/athena/TaskList.java#L179-L189

Empty lines

Remember to add these files to the .gitignore

I think expected vs actual would be a more consistent naming since this was used for our IO tests. But this is fine too.

Other than that, since this is a BufferedReader object, it would be nice to have the variable name sound like it, for example answerFileReader or expectedFileReader? Same goes for attempt in this method.

else is not needed since this will always be reached when the if condition is not satisfied.

This shouldn't throw an AddException. Maybe a CommandException?

getErrorMessage gives an impression that it is returning something. Perhaps printErrorMessage instead? Or return the error message as a string from this?

Also remember to add Javadoc comments for this

1 empty line should be enough?

I feel that it would be better to have a more specific exception name.

It is not clear what toggle does.

Can we use TaskList instead? So that we can make use of methods from TaskList if needed.

Can we call this TimeAllocator? A noun instead of a verb since it's an object.

Ui the i is lowercase in our code.

isFlexible according to coding standards

isFlexible

isTaskFlexible?

isFlexible?

isFlexible? The program still works when there is a method called isFlexible

isFlexible?

How about something like loadTaskListData_commaInTaskAttribute_commaIsReplaced so that it is more descriptive?

and I think it is good enough to use "TODAY" instead of Recurrence.TODAY.toString()

What does 5 refer to?

I think it is better to override the clone method that is in Object.


    @Override

    protected Object clone() throws CloneNotSupportedException {

        return super.clone();

    }

getDatesAt instead? Date in plural.

Also, should we be using getTaskFromNumber instead?

getFirstDay is not so clear. Perhaps getFirstDateMatchingDay? Or something more clear?

I think you can just do recurrenceDates.clear()

removeDate instead?

"dd-MM".length() instead? DATE_TIME_FORMAT sounds quite confusing.

I suppose we should use the next year if the date has already passed. For example 1 Jan should be in year 2021.

Unused import?

I think can use task.isFlexible() here, which has clearer intention than task.getDates() == null.

Can this be isDateIncluded instead?

I think filterDates is not very clear. How about something like removeExcludedDates?

The isFlexible attribute of this class is to determine whether to get flexible tasks or not flexible tasks. So, this needs to be task.isFlexible() ^ isFlexible. But anyways I think isFlexible is not very clear. How about something like filterFlexibleTask or doesIncludeFlexibleTask

Change to isDateIncluded too?

Remember to remove this because it's no longer used.

I think it is unclear to say today, since today means only TODAY in our program? Something likethisDayOfWeek instead?

Same as above for today.

Slight nit: testRemove instead of testremove.

I also think day could be more descriptive, so that from the name we can tell what the test case is testing. How about filterToday or something that shows it is testing for filtering based on TODAY?

Also the method doesn't return true so this is not very precise. How about returnsOnlyTodayDate?

Same goes for the methods below.

Is this supposed to be Forecast.WEEK instead, according to the comment?

Unused method? It doesn't seem to do exactly as the method name suggests too.

Also remember to add a test for the exception.

I think toStringDetails sounds weird. How about getDetails() or getDetailsAsString()?

How about the name taskDetails instead of task? Because we always use task for Task objects.

I think we should refactor this into another method.

Should commandAndDetails just be commandType?

parsedCorrectly should start with lowercase

lowercase parsedCorrectly

I think if the user inputs l3 f/WEEK, the shortcut won't be expanded, and then the command will fail. I think parseCommandType and parseShortcutCommand can be combined together since they look at the first word in the command.

Need a space after the * for them to show as bullet points.

Oh since we're not using it anymore let's remove it.

Allocator should start with lowercase a

Why does this throw an Exception?

Since this is a class/object I think it would be better to name it with a noun. Something like TaskTimeComparator? I think that this also helps us immediately tell that it is a comparator when looking at the name.

Was this for testing?

Missed this out last time. Since fixedTask and flexibleTask are lists, their name should be in plural form as per the coding standards (fixedTasks and flexibleTasks).

After #131 it will be in LogicManager.

Lol you forgot to finish? Or we continue this next time?

Strictly speaking this shouldn't be part of this PR. But this is good.

the best theoretical timetable...

I feel that theoretical will raise some questions from the reader (aka Aileen). I think we can remove this word.

Wanna give example commands?

theoretical same as other comment

It splits the user input based on command type...

based on the command type instead of based on command type?

Just a suggestion: I think it is easier to read if we say "the user can provide the parameters in any order" in a separate sentence.

I think it is not clear what It refers to. How about mentioning Parser, AddCommand and TaskList.

calls the AddCommand class

"creates an AddCommand object" would be more precise?

Writes the current task into...

Should be "Writes the current task list into"?

The input will be parsed into LogicManager

passed instead of parsed?

which executes the AddCommand class

creates an AddCommand object would be more precise

returns a boolean value "false" to the active flag

I think false can be written like false because it is code.

And we got an active flag?

What are these?

I think we should name it as something that sounds like a Comparator, or at least a noun. Something like TaskImportanceComparator?

Similar to the other comparator, something like TaskTimeComparator?

How about


if (taskFilter instanceof ForecastFilter) {

    ...

}

else {

    filteredTasks.add(task);

}

since the other filters don't need to do anything.

Unused import?

flexibleTaskList and fixedTaskList since they are tasklists?

Is this used?

Can use assertEquals

This sounds like it's for setting today's date. But I think the intention here is to change the date that is to be filtered? I think we should make a new Forecast value for this purpose instead of reusing TODAY because it can be quite confusing. In that case, I find filterDate to be clearer than todayDate too.

I realized that when the Forecast is DAY then it must have a date. So, it would be good to have it part of the constructor. How about having a second constructor that takes in a LocalDate, which sets the forecast to be Forecast.DAY.

Remember to remove this.

Shouldn't it be timeInfo.getStartTime().getHour() in the set? Otherwise the loop seems to be pointless.

It would be good to refactor this code segment into methods to maintain single level of abstraction.

nit: int duration instead of Integer span to keep things consistent, since we normally use int for integers?

temp as an argument name/being part of a method name is probably not a very good idea

Remember to remove

Why can't we just remove the task right here?

Would be good to refactor this segment to maintain a single level of abstraction.

Suggestion: Can do undefinedTimeTasks.isEmpty()

I think it would be good to make a variable for end - space - pos otherwise it takes some brain power to know what this part does exactly.

I thought that count would need to start at the starting hour that temp started at?

Would be good to refactor this part into methods to maintain single level of abstraction.

Is this supposed to throw a TaskNotFoundException? If this throws a TaskNotFoundException, the user will receive an error message saying the task cannot be found. I think this would be really confusing to the user.

How about something like spaceSlots instead of temp?

I think date is only meaningful when forecast == Forecast.DAY so it might be better that we don't accept a forecast argument.

Is it supposed to be day > 31?

Suggestion: How about nextVacantSlotStart or something that indicates that this finds the start of the next vacant time slot?

Suggestion: How about nextVacantSlotEnd or something that indicates that this finds the end of the next vacant time slot?

Remember to remove.

I think it would be better to say that this method will store the tasks that did not get assigned yet into this parameter. Otherwise, it seems like the caller has to provide said tasks.

Suggestion: active doesn't clearly describe why the loop has to continue running, a name that describes so would be nice

taskNumber from line 161 is unused.

I think that for this one, it would be good to catch the InvalidCommandException, to ensure it is this specific exception that is being thrown.

I believe the test should fail if the this part fails, so we shouldn't catch and handle this exception.

I don't think we should print the stack trace to the user at all (as done by e.printStackTrace()) no matter what happens. We should give an error message saying the program failed to load the file. Perhaps we could make our own exception like StorageLoadFailedException so that we can customize its behaviour too?

Actually, I realized ui.printInvalidTask is ambiguous to the user too. The user wouldn't know which task is disposed and this is not really a good thing. I think it would be good to change this in this PR too.

https://github.com/AY2021S1-CS2113T-W12-2/tp/blob/4d422985a6a9c8baadd0397b5c10a979bbb5267b/src/main/java/athena/Ui.java#L134-L139

Also it is not immediately obvious that ArrayIndexOutOfBoundsException occurs when the save file is corrupted. Perhaps we should catch this exception in Storage and throw a custom exception like StorageCorruptedException.

StorageException instead?

Why so many empty lines?

This should be a heading?

Got an extra space in front of TaskList

Suddenly there is a getLog that came out of nowhere. Would be good to explain it.

Full stop at the end

Do we need to check the existing tasks?

Shouldn't existing tasks not go past the 12am mark? Wouldn't we have stopped that from happening when it was added/edited?

Unused constant?

test?

If we do getDayOfWeek means it will occur every week right? Should it be just LocalDate.now()? Btw if we want to get the string representation of a LocalDate, remember to use DateUtils.formatDate because toString doesn't return the same format as what we mention in our user guide. This could also be a reason why we get the error message I don't know the date..., because I realized this kept getting printed in the tests when we use date.toString.

https://github.com/AY2021S1-CS2113T-W12-2/tp/blob/9e9ddbb751b86b381b84f0ee3a2455e2addd9f66/src/main/java/athena/common/utils/DateUtils.java#L68-L71

@alstontham I think it would be good to overload the toString method so that we can print the object directly. You can work on printing out a Date and TaskList on your side first then we can move that code here later.

Or you can fetch this branch and modify it, then make a pull request to my branch.

Yea I tried without and the program might crash if the user doesn't give it in upper case.

I agree!

Oh wait, one allows the programmer to choose the index while the other will automatically assign an index. But yea there's repeated code so I'll modify the second addTask to call the first addTask.

Actually I wasn't sure should the number start from 0 or 1. I thought our first task would start from 1?

This was taken from #55. Can you comment over there?

How about moving number to the first argument for addTask as well?

Oops thanks for noticing

Oh right good catch

I removed the runtest.sh from GitHub actions in 732c687 because I realized this is not practical.

Oh I was planning to refactor ------- into a constant. Will do that tomorrow.

It turns out that the one in 115 was looking for the first date that matches the day, e.g. given Wednesday return 21 Oct 2020. So it's different from this. But yea I agree it would be better for this to be somewhere else, so that it can also be commonly used by the date related tests.

Nope since the checks passed

At first I was thinking of putting it in Ui, and then I feel that eventually we would need to refactor them to somewhere more focussed. Then I realized the somewhere is Timetable, since it contains all the related attributes needed to draw the timetable. What do you think?

Ok will leave this to you

Oh thanks I forgot about that.

@wish2023 Can check if the yyyy-MM-dd was a mistake?

Done in #4

@Kafcis I think we don't need to store the timetable generated from the task list in a file, because the same timetable can always be generated from the same task list. Looking at the scale of this, there won't be much impact in performance if we generate the timetable when the program starts. But if you wanna do it, you can use getTimetable() to get the week's timetable.

@alstontham The timetable given to the Ui will be a list of TimetableDay objects, each containing a date and a list of Tasks. So you can work on printing that now.

Can you add a method to get the whole list without any filtering?

Actually there's no checkstyle issues here lol

I feel that this can be just a method inside Task. Have we discussed this?

Done in #34

Er this PR has error with checkstyle test after you merged master into it. We shouldn't have merged.

Yea if main throws the exception it's the same as asking the program to crash because nobody is handling it.

Done in #45

Btw merge from master so that the checkstyle problems are fixed.

Thanks. I agree it would be better to have a Recurrence class so that we can support different types of values.

Also continuing the work in #59.

I just tested and realized sending exit doesn't actually exit the program. You forgot to check for that.

Continued in #61

I merged #55 into this so that I can also catch exceptions. We can wait until that is merged before reviewing this if there are too many things.

I think each change in the code should be accompanied with the test code so that we can merge with confidence that it works as intended.

My bad now it is fine, not sure why it crashed just now. I should have checked what went wrong exactly.

I think this would fix #73. Can confirm?

Left with AddCommand

Test is failing because of ForecastFilterTest. Should be ok once #78 is merged.

Done in #79

Also closes #69

@wish2023 ForecastFilterTest dieded again

We should eventually get rid of this sloppy exception handling.

https://github.com/AY2021S1-CS2113T-W12-2/tp/blob/7d0912d0fa6204dc74fc2f102ac7f402ee315eb4/src/main/java/athena/task/Task.java#L66-L73

Did we fix this?

There would be a problem when the task details contains a |. I think commareplacement is too long if we want the save file to be conveniently edited by the user.

Fix #98

@wish2023 I'm thinking of merging this first so that we can move on with the other PRs. Will open another issue to address the comments you left. Ok?

We should also test that 7 days in the week (Monday to Sunday) are included when filtering by week. And address the comments left in #99.

Can we close this? Done in #114

No longer a problem

Can close? Done in #96

Some work done in #96 and #115

Can be done together with #103

Remember to fix the failing test too.

This is done already?

Good catch. Currently if there are overlapping tasks, the second task will not show up in the timetable. I think we should leave this to the TaskList and TimeAllocator to ensure this doesn't happen? Because I don't think we should allow this to happen at all.

Btw for now, if the task duration exceeds the sleep time, it will be cut off at the sleep time. Perhaps we should push back the sleep time when this happens? We can open an issue for this if so.

Ya it shows for the week for now. After this is merged I will let it be based on the ForecastFilter (#129).

We might need to postpone to v2.1 because our OOP is not really good enough to allow us to store this information anywhere, since our Timetable is instantiated on the spot when list is called. We would need a Model class that interfaces to all our backend stuff like TaskList and Timetable before implementing this.

Now we cannot do this because we are loading the Task attributes from the TaskList and filling them into the EditCommand. We can change Task.edit to instead check if the method arguments are null, and only modify the Task attributes if they are not null. By doing this, we can improve testability since EditCommand no longer needs to load the attributes from a Task.

Resolved in #142

In DG it says ColourManager but in code we have Color.

Oh ok no prob

Don't allow user to add tasks from 12am to 8am.

Where are the diagrams lol? This PR only has the code for generating them.

Combination of #169 #177

Done in #178

Is this fixed?


    public Resistor(double value) {




    /** 

     * Returns String representation of Resistor.

     *

     * @return String with value and SI unit of Resistor. 

     */


    public String toString() {

Great photo!

Nice work!

If you look in the build error, it says 'METHOD_DEF' should be separated from previous statement. [EmptyLineSeparator]. So just have to add an empty line.

/**

Consider correcting the SI_UNIT?


    private static final String SI_UNIT = "Ω";

Need to extend the super class.


public class PowerSource extends Component {

The JavaDoc should be with its method.




    /**

     * Returns String constituting value and unit of voltage source of circuit.

     *

     * @return String representation of VoltageSource object.

     */

Consider JavaDoc here?

Might be good to have getters for these.

Later when the RTemplate is created this could instead inherit from that.

Same, a getter for this could be used later?

Consider moving the static constant to before the instance variables?

https://se-education.org/guides/conventions/java/index.html#classes-and-interfaces

Not needed since done in super() call.

Consider removing the line between the JavaDoc and method? From the coding standard: https://se-education.org/guides/conventions/java/basic.html#comments

No blank line between the documentation block and the method/class

Could consider having setters for these? @sevenseasofbri LrTemplate could use them as well?

Can also add JavaDoc.


    public double getImpedance() {

        return angularFrequency * Math.abs(inductor.getValue() - (1 / capacitor.getValue()));

    }

You could consider to just return here and let the rest of the function get carried out instead of having else.


            ui.printSetVoltageSource(template.getInitialPowerSupply());

            return;

Because of inheritance this is not needed.

Maybe the assertion could be not throwing an exception? This would probably be better checked in the test for the LcTemplate.


        assertDoesNotThrow();

Ah ya that's what I meant.

With the return, no need for the else here actually. So it can just be done after the first if statement.

Consider updating the method name to be more specific? Like https://se-education.org/guides/conventions/java/basic.html#naming

featureUnderTest_testScenario_expectedBehavior()

Also can consider adding edge cases? Like 0 or negative values to make the tests more robust?

Perhaps the addSeries() and addParallel() methods could be tested as well? They are specific to the LoadComponent subclasses.

Oh that's a good point. No need to put them here then, you can just add test cases to LoadComponentTest?

Could else throw new DukeException then?

Yep looks good!

Since execute() actually takes in a Ui object, you can consider using Ui.readline() instead.


            command = ui.readLine();

Consider leaving System.out.println() in the Ui class?

Consider leaving this in the Ui class?

The help and summary command should probably have text-ui-test before merging.

Oh this try-catch block is probably not needed since all DukeException are caught in the main Duke class right?

Alright noted then, it's like a mini instance of Duke.


        }

        

        if (numOfCommandsDone >= 6 || !command.matches(orderOfInstructions[numOfCommandsDone] + "(.*)")) {

            throw new DukeException("Bad Command! Please follow the instructions carefully."

                    + "If you want to exit this mode, type 'exit' and press enter.");

        }

        

        Command c = PARSER.parse(command);

        c.execute(ui);

        numOfCommandsDone++;

What's the reasoning for making this (as well as in the other classes) protected instead of private?

Probably don't want to XOR here right?


    public static final double DELTA = 1e-15;

Thinking the return toString methods could simply be tested in text-ui-test. Kind of odd that we just copy paste for JUnit.

Consider changing all method names to include the test input/scenario.

https://se-education.org/guides/conventions/java/basic.html#naming

Consider adding edge cases? Like 0, or negative values, to make the code more robust - similar for all the methods.

i.e. test scenario here could be noValue

Ah okay, but I would say there is no need to access the TEMPLATE (see comment about the toString), and the components could be accessed by getters instead, so actually don't need to be protected?

Trivial getters wouldn't really need testing in the end: https://stackoverflow.com/questions/6197370/should-unit-tests-be-written-for-getter-and-setters

Since the way we set values is calling Component.setValue(), these actually still work when they are final.

In a sense, we are using the same LoadComponent object, just changing its value.

Looking at https://github.com/se-edu/addressbook-level3/blob/master/src/main/java/seedu/address/logic/LogicManager.java, I think it is completely fine to have the Logger at the main Command class. It's just that we haven't thought of logging for the other commands, but I think this should be moved to the main class.

Nice use of abstract!

Any reason why the @param input here is of byte type when everything else is int? Including that of TwoInputGate.setSecondInput().

Probably clear enough to return immediately.


        return input & secondInput;

Should this check for isEmpty() first?

Nice example!


        boolean hasNoChildren = isNullAtIndex(getLeftIndex(index)) && isNullAtIndex(getRightIndex(index));

        return !isNullAtIndex(index) && hasNoChildren;

4 could be a constant defined above.

I don't think we need that. Reason being we are not guaranteed the inputs are set when building the circuit. Might have to calculate every time, unless BooleanTemplate checks for if a new value is set or not.

Any Gate would also need a default overloaded constructor in that case.

Do remove the conflict markers.

No need to remove.

You feel that no need to put the User Profile and Value Proposition to the ToC?

Consider using bullet points here? Possibly standardise the numbering/bulleting in this Appendix section?

Nice diagram!




![InitialBinaryTree](diagrams/BinaryTreeInitialObjectDiagram.png)


`BooleanTemplate` uses this function to render the current configuration of the circuit in a String format. The method

is also extensively used in other internal operations in `BinaryTree<t cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2113\tp-comments-panels.mbdf">` for checking whether a position in the tree has been set or not.

Just realised this shouldn't have been deleted.

Should be


## Appendix: Requirements

### Product scope

#### Target user profile

This has been changed to CLIrcuit Assistant right?

Is this a copy of the main README?

Currently broken.


## Appendix: Requirements

The jar file is still duke.jar.


2. Open a new terminal window and navigate to the same directory where your duke.jar is located

3. Enter the command `java -jar duke.jar` into the terminal window to launch the application

Just the link for changes in #73.


    * [Model component](#model-component)

* [Implementation of Boolean Commands](#implementation-of-boolean-commands)

    * [Binary Tree](#binary-tree)


[-> AddCommand: execute()

RcTemplate calls getCapacitor() from itself.


RcTemplate -> RcTemplate : getCapacitor()

activate RcTemplate



RcTemplate <- RcTemplate : capacitor

deactivate RcTemplate


AddCommand -> RcTemplate : rc.getComponent("c")

I realise that we might need to change the actual Java code here so it doesn't need to call getComponent() twice.

This whole part might actually be redundant in the code, we can remove the setComponent() call since it is already set when c.setValue() is called.

I think you can just update the UML to this version, we can merge and create a new PR for the code later. Maybe can just add an issue right after merging this one.

Yeah maybe can add to .gitignore.

The code has been fixed as of #71 - you can update it to follow the current implementation.

You can click delete file from the Files changed section.

This would probably be better moved to its own section rather than being in the Architecture section.

I'll have to update my diagram.

Remember to update the ToC once you do so 💯


/.settings/

*.log*

Interface with only a constant?

Why is this needed?

Duplicate

Should not be invalid command, need to change input.txt to use tutorial circuit instead.

Maybe can add JavaDoc here?

I see, will have to change the class diagrams for this.

Why is this necessary? Doesn't seem to be used outside of checkHasSetBothInput().

JavaDoc here?

Following CircuitCommand, what could be done instead is to put this in an overridden toString() method:


    }

    

    @Override

    public String toString() {

        return "The output of the current configuration is " + template.calculateOutput(0) + ".\n" + super.toString();

    }

Better yet, have a private int output variable in the class, and use .execute() to set it using output = template.calculateOutput(0). Then you could replace it with output in toString().

Yep can remove.

This is no longer necessary, along with the import.

Consider initializing the Gate object before these if checks? Seems to be circuit.getT(idx) is accessed up to 3 times.

This could be overloaded so that calculateOutput() will just give calculateOutput(0) when called from CalculateBooleanCommand. Then this method could also be private.

No new Gate object is being created, you are accessing an existing one within BinaryTree. Plus you get much more clarity.

On that note, you could also have a variable for the left and right index.


        return super.toString() + System.lineSeparator() + "The output of the current configuration is " + output + ".";

Following the implementation this should above the template.

Nice addition! 💯

Any reason why this doesn't use the CLIrcuit Assistant logo? And Enjoy being on the same line? Consider using EXPECTED.txt and ACTUAL.txt, which you'll have to update, to check the formatting.

Adding on to this, how come there are two sets of DukeException being thrown?

Consider putting this in CircuitTemplate instead? Then subclasses can just call super.toString().

Will be updated by @oasisbeatle

👍


+-----------+----------------------------+---------------------+

|  Command  |         Circuit            |      Boolean        |

+-----------+----------------------------+---------------------+

| Tutorial  | tutorial circuit           | tutorial boolean    |

| Template  | template TEMPLATE          | template TEMPLATE   |

| Summary   | summary                    | summary             |

| Set       | set COMPONENT VALUE        | set INPUT BOOLVALUE |

| Add       | add CONFIG COMPONENT VALUE | add INPUT GATE      |

| Calculate | calc EFF_VALUE             | calc                |

| Exit      | exit                       | exit                |

| Print     | print                      | print               |

+-----------+----------------------------+---------------------+



Here's a summary of the command parameters: 

+-----------+----------------------------------+-------------------------------+

| Parameter |       Circuit Params             | Boolean Params                |

+-----------+----------------------------------+-------------------------------+

| TEMPLATE  | rc, rl, r, cl                    | boolean                       |

| COMPONENT | r, c, l, v                       | N.A.                          |

| CONFIG    | parallel, series                 | N.A                           |

| EFF_VALUE | reff, ceff, leff, current, power | N.A                           |

| VALUE     | any number of double datatype    | N.A                           |

| INPUT     | N.A                              | A, B, C, D, E, F, G           |

| GATE      | N.A                              | or, and, nor, nand, xor, xnor |

| BOOLVALUE | N.A                              | 0, 1                          |

+-----------+----------------------------------+-------------------------------+


You have exited Tutorial Mode!

Here's a summary of available commands:

Why are the stars added?

Do correct the name of the image file.


#### `SetBooleanCommand`


There are four Boolean commands that are used in the implementation of the logic gates: `TemplateBooleanCommand, SetBooleanCommand, AddBooleanCommand`, and `CalcBooleanCommand`.


#### `AddBooleanCommand`


#### `CalcBooleanCommand`

Where is the PUML file for this? It seems like the TwoInputGate objects shouldn't know about it each other, rather it's BooleanTemplate that knows about all of them.

Missing PUML file.

Going into BooleanParser from Parser should be prepareBooleanTemplate().


[->Parser: parse("template and")



activate Parser



Parser-> BooleanParser: prepareBooleanTemplate("and")


BooleanParser --> BooleanParser: AndGate()


BooleanParser --> BooleanParser: BooleanTemplate(AndGate())

The BooleanParser is not deactivated.

The control should be returned back to Parser, that's where it started from. We can just ignore the super() call, it's not so important.


TemplateBooleanCommand --> BooleanParser

deactivate TemplateBooleanCommand 



BooleanParser --> Parser

deactivate BooleanParser



[&gt;-- Parser

deactivate Parser

This is still not fixed. Do change the name of the image file to SetBooleanCommand.png.

Do keep in mind the names of the image files.


![InsertObjectDiagram](diagrams/AddCommandOrAndXor.png)

Please fix the activations, they should all be after the arrow ->.

Index parsing is done in BooleanParser.


BooleanTemplate -> BinaryTree : insert(idx, and)

activate BinaryTree



BinaryTree --> BooleanTemplate

deactivate BinaryTree



BooleanTemplate -> BooleanTemplate : buildTopDown()

activate BooleanTemplate



BooleanTemplate --> BooleanTemplate

deactivate BooleanTemplate



BooleanTemplate --> AddBooleanCommand

deactivate BooleanTemplate



&gt;-- AddBooleanCommand

deactivate AddBooleanCommand

This will need to be revised once @sevenseasofbri implements CalculateBooleanCommand.execute(). Consider revising the return of control of the sequence diagrams.

Please fix the activation here as well.


[-> SetBooleanCommand : execute()

activate SetBooleanCommand



SetBooleanCommand -> BooleanTemplate : setInput(template, 2, 0)

activate BooleanTemplate



BooleanTemplate -> BinaryTree : getT(2)

activate BinaryTree



BinaryTree --> BooleanTemplate : gate

deactivate BinaryTree



BooleanTemplate -> TwoInputGate : setSecondInput(0)

activate TwoInputGate



TwoInputGate --> BooleanTemplate

deactivate TwoInputGate



BooleanTemplate -> BooleanTemplate : buildTopDown()

activate BooleanTemplate



BooleanTemplate --> BooleanTemplate

deactivate BooleanTemplate



BooleanTemplate --> SetBooleanCommand

deactivate BooleanTemplate



[&gt;-- SetBooleanCommand

deactivate SetBooleanCommand

Needs revision - consider changing this to BooleanTemplate being associated with Gate objects, which all the other gates inherit from. Also, TwoInputGate only has input and secondInput, they do not know about other Gate objects. The logic is done from within BooleanTemplate.

Maybe this can be better shown with a UML Class diagram?

Maybe these steps here and the next command can just be shown by UML diagrams mostly? If the sequence diagram is very clear, the steps taken might even be redundant.

The name of the actual file is still docs/diagrams/setBooleanCommand.png. It will not render properly if the case does not match.

Any reason you name them States? Seems harder to deal with than just using their names.

We no longer have the TwoInputGate, all are from Gate now.

Consider revising these to show inheritance i.e. the >|-- arrow.

Arrow is in the wrong direction?


State1 --> "1..7" State5

The "1..7" shows the multiplicity.

Already implemented - can be updated.

No longer needed. Also States name could be changed?

Yes.


Nice, added a 500.0 µF


![InitialBinaryTree](diagrams/BinaryTreeInitialObjectDiagram.png)


import java.util.Scanner;


    public static void main(String[] args) {

actually this was the old tutorial commands, can remove these then

lines 10 to 16 were the tutorial commands previously. see the exit command?

yes I know. but we have 2 sets of tutorial commands for tutorial circuit now.

Nice work! I admire the beauty of this change.

Adding an edge to an AM is O(1), checking if an edge exists is also O(1). Removing an edge is also O(1). The issue with the AM is the high space complexity.

Since we have kramdown with {\:toc}, this isn't needed right?

I think we are all forgetting the GUI constraint here. Any dynamic visualisation would dangerously be part of GUI, and making everything static (similarly for Boolean) cuts that issue out. Maybe consider adding this consideration as well?

You can take a look at our current documentation page which already uses it: https://ay2021s1-cs2113t-w13-3.github.io/tp/DeveloperGuide.html

If you want to see it on your own fork you can setup the docs as the site page in your settings: https://nus-cs2113-ay2021s1.github.io/website/schedule/week7/project.html#a-userguide

Will be used when it's actually implemented. The Ui will need to print what happens with execution, something like my iP. i.e. the implementation here is not complete, and requires #3 to be completed.

Hmm, I feel it's quite intuitive? What else would you do with the otherValue, with the method name and JavaDoc already given?

Nope! In fact, it cannot compile if you add the break statements.

We can consider adding more Exception classes and functionality later on.

Good idea 👍

Good point. Should there be an assertion in BooleanTemplate.setInput() as well? Though it does seem we are using DukeException for BinaryTree.insert(). Maybe all as exceptions?

Yep yep, I basically left this whole part untouched since @PraveenElango should be solving it in #75.

Same as above.

#10 Inductor done.

No need to link the issue #10 yet, else it will automatically close it when you merge. Can link when the last component is done.

Closed as duplicate of #19.

You don't need to close the PR and make a new one once you've made the changes. But it's okay, now that you've opened #19 we'll use that.

Inductor done in #11
Capacitor done in #16
Resistor done in #19
Voltage Source done in #21

I think we can merge all and close this issue?

Todo:

Take a look at Parser.prepareCalc().
Take a look at Command and other subclasses.
Create CalculateCommand class.
Implement CalculateCommand.execute() method.
Use Template calculate methods to get values.
UseUi to print result.
Add JUnit test(s) for CalculateCommand.

Assigned JUnit Testing classes:

| Class | Person |

|-------------|--------------------------------------------------|

| Command | @sevenseasofbri will do HelpCommand subclass, @dorianfong98 the rest |

| Component | @oasisbeatle |

| Parser | @hughjazzman |

| Template | @PraveenElango |

| Ui | @sevenseasofbri |

Being the last task of milestone v1.0, we can follow this after closing: https://nus-cs2113-ay2021s1.github.io/website/schedule/week9/project.html#using-milestones

A working product tagged.
CI passing.
Milestone updated to match the product.
Milestone closed.

@dorianfong98 Command classes done?

To be updated with #57 features, including the user stories addressed.

@sevenseasofbri will update Glossary once everyone has updated the Dev Guide. #73

Duplicate of #62.

@dorianfong98 to do BinaryTree

@sevenseasofbri to do BooleanTemplate

Add a PrintCommand to print both CircuitTemplate and BooleanTemplate.

@sevenseasofbri Add the parsing into Parser,BooleanParser and CircuitParser, to return TutorialBooleanCommand and TutorialCircuitCommand.

On these two points:

Why it is implemented that way.
Alternatives considered.

I believe we can add this for the CircuitTemplate as well as the BinaryTree.

Can @oasisbeatle or @dorianfong98 do a short write-up for CircuitTemplate and @sevenseasofbri or @dorianfong98 do one for BinaryTree?

We can also add future (hypothetical) features past v2.1.

Not implemented in v2.0.

Released: https://github.com/AY2021S1-CS2113T-W13-3/tp/releases/tag/v2.0

Hi Nishanth, thanks for the review! Is it ok if you check out the current PR #119 DG to see if some of these issues are resolved?

https://hughjazzman.github.io/tp/DeveloperGuide.html

If the LrTemplate() call is a constructor, the arrow should point to the :LrTemplate object, and the activity bar should be connected to :LrTemplate. Please review the other sequence diagrams and constructors as well.

@sevenseasofbri I looked through the plantUML docs, I think you can use the 'create' keyword for object creation

@nishanthelango Ah I should've looked too, makes a lot of sense that constructors can be called that way within plantUML. Thanks!

(Figure 16) Consider removing the parentheses in AndGate() to avoid confusion with methods

@nishanthelango Is this perhaps clearer?

Yep won't be added.

Thanks for the feedback. Will fix, but will not appreciate the rudeness of this issue. Do provide feedback without sarcasm and more tact. @brandonywl

The presence of a table to summarize keywords is great but other than code, the rest are only used 3 times or irrelevantly.

I disagree. Dr Jane (CS2101 prof) has informed us that this is a good way to make sure there is no confusion when different types of formatting or icons turn up in the guide. Maybe you can explain the rationale as to why they are irrelevant?

Irrelevant use of bolding. Provides no significant information to user.

Would still disagree with this point though, since CLIrcuit Assistant is the name of the application. Maybe you can make your point with a proper explanation?

For code, the bucket is too broad and confuses users as to what they should see and what they should type into the CLI. It is preferable to use screenshots instead for what they should see and that should reduce your issues by quite a fair bit.

Fair enough, I suppose there should be differentiation between only user input and formatting command words.

Unclear which should I actually put into my CLI or should I see. Surprise! It's none of the two. Only the last line follows the standardized guide. This is repeated consistently across the UG.

It's unclear which two you are referring to here. The title/section headers are definitely not equivalent to the format, which admittedly could be explained more or with better formatting. As for the Tutorial commands, perhaps we could add a preface that the Tutorial commands should be followed to the T, and that the User Guide is only a general overview. Also, another preface to explain that all user input are under the Format: (maybe Format could be bolded or underlined) with the Expected Output being directly after could be placed before the commands.

This change would be under the SetCircuitCommand#toString() method actually.

It seems that following the original format given to us wasn't too useful. I suppose we could put the format in the section names themselves.

Disagree here. Link is provided from the top for the summary of commands at the bottom. However, I do believe the Command Preface could include this fact.

Similar to #129.

@PraveenElango I think straight away can print the summary table, and have the parameter command stuff only in the DG.

Is it possible to catch the exception with this function and return a new IncorrectCommand("Missing info" or "No keyword") here

I am thinking that the printFindNotesMessage in InterfaceManager can be a static function and it returns a string of the formatted note. This is to consolidate all the printing to be only done inside the main runLoop function

Make this function return the string instead of printing them out directly

Can use InterfaceManager.LS (line separator) for the \n to standardize all new line

since you are using get function, maybe can change all the Strings to private type in the respective class

I think you can import these from the tagManager.

The person has to enter the prefix for every tag they want to add so I think can leave it as "TAG" to avoid confusion.

Thats also viable, also can add in "TAG COLOR" too

Don't have to make it static, each Command class have a TagManager object, can access the tagManager from there

Get from the tagManager instead.

the name of the variable can be COMMAND_WORD since now add event is split into another class

Can replace the "Note deleted:" with your command successful message

Violates coding standard

Likewise violates coding standard

Nice function here.

Dont need to have this catch here. Make this function throw a SystemException, the parseCommand function will catch it.

Should there be some error handling here? Like incase there are duplicate events.

Can rename COMMAND_WORD_EVENT to COMMAND_WORD, previously used this to separate COMMAND_WORD_NOTE but since now this has it's own class can just shorten it.

You are also missing the COMMAND_USAGE_MESSAGE

I used the wrong "Boolean" when creating the class, can change it to "boolean"

Makes sense.

prepareAddNote (and the other prepare functions) do not have the catch block for SystemException. This catch will be handled inside parseCommand. The only exception to catch in prepare functions should only be NullPointerException, ArrayIndexOutOfBoundsException and maybe date time related exception for this function.

Can make this private and have a getter function for it to be used in HelpCommand

Can replace the \n with InterfaceManager.LS for consistency.

throw new SystemException instead of return a new IncorrectCommand. And can remove the catch for SystemException.

There should be a catch for NullPointerException in the event that the user just enter the command only without any space after it. i.e. "add-e"

This two should be private members.

This function can be moved into Parser. The original intention of this class is to create a list of acceptable date time formats and returns date time info in a specific format.

There should not be any return new IncorrectCommand here as well as the catch block for SystemException. Can change the catch block for ArrayIndexOutBoundsException and NullPointerException to throw a new SystemException.

Suggest to split the catching of the two exceptions mentioned above to throw a more specific SystemException.

Prepare functions should throw SystemException if necessary.

The exception should be missing index prefix instead for your case.

Would it ever reach this point? If so, can change it to throw SystemException instead of return new IncorrectCommand for consistency with other prepare functions.

Actually you can return a new DeleteNoteCommand within the switch case. So it should return immediately after reading a title or an index. If it reaches the code after the switch case can throw a systemException, say missing title/index or missing description

Is it necessary to create a new Note object here?

Maybe can make this a static final string as COMMAND_UNSUCCESSFUL_MESSAGE

A space after the comma

Have you tested listing notes with a non-existing tag? i.e. list notes with CS2113 tag, but there is no such tag in the map at all? getTag returns null for such case so I think you should only add those that are not null.


            Tag tag = tagManager.getTag(tag);

            if (tag != null) {

                tagList.add(tag);

            }

Can do a check for the size of notes, if it has no element can return an unsuccessful message.

This can be private since its only used within this class.

Correct me if I am wrong but I don't think JavaDoc works inside function? Like it won't be displayed on the html.

p.s. I am referring to Narzyl comment.

Is there a need to append to the noteString or can you just return the unsuccessful message?

I think it would look nicer if the tags are on the same line as the title. i.e.

CS2113 Notes [CS2113] [Important] [School]

Shld this comment be above? or somewhere in execute? Looks a little out of place


    private static final String INDEX_OUT_OF_RANGE_MESSAGE = "The index you specified is out of range. "

            + "Please specify the index that is indicated when you print the event list";

For the "Year-Month(1-12)"

Is the user suppose to enter the year followed by the month, 2020-1, or its just the month itself?

I believe Ramana is renaming all the exception with "WRONG" to "INVALID", can follow that.

Rename as event? Preferably not to use single letter for variables unless its for iterators.

The parameter has to be an Object? Is there any instance that this will be used to compare non-Reminder objects?

Not to sure about this, but is there a need to create a class for this? Does implementing to a class, say Event or DateTimeManager and then overriding in that class work? Like wise for the DateTimeComparator, since a class can implements multiple interfaces.

Can declare these numbers as static final variables

I'm not sure if I remember correctly but previously Ramana mentioned that can return a new RemindCommand() directly right, since you are not using userMessage

Don't have to create new object for all. In your case can just create Timetable, and pass in null for the other parameters


        return Formatter.LS + COMMAND_SUCCESSFUL_MESSAGE + noteString.toString();


            return Formatter.LS + COMMAND_UNSUCCESSFUL_MESSAGE_INVALID_TAG;


                return Formatter.LS + COMMAND_UNSUCCESSFUL_MESSAGE_EMPTY_NOTEBOOK;


            return Formatter.LS + COMMAND_SUCCESSFUL_MESSAGE + noteString.toString();


    private static final String NOTUS_LOGO = Formatter.LS

'''suggestion

        + Formatter.LS

        + "     .-') _               .-') _                 .-')    "

        + Formatter.LS

        + "    ( OO ) )             (  OO) )               ( OO ).  "

        + Formatter.LS

        + ",--./ ,--,'  .-'),-----. /     '._ ,--. ,--.   (_)---\\_) "

        + Formatter.LS

        + "|   \\ |  |\\ ( OO'  .-.  '|'--...__)|  | |  |   /    _ |  "

        + Formatter.LS

        + "|    \\|  | )/   |  | |  |'--.  .--'|  | | .-') \\  :' '.  "

        + Formatter.LS

        + "|  .     |/ \\_) |  | |  |   |  |   |  | |( OO ) '..'''.) "

        + Formatter.LS

        + "|  |\\    |    \\ |  | |  |   |  |   |  | | '-' /.-._)   \\ "

        + Formatter.LS

        + "|  | \\   |     ''  '-'  '   |  |  ('  '-'(_.-' \\       / "

        + Formatter.LS

        + "'--'  '--'       '-----'    '--'    '-----'     '-----'  "

        + Formatter.LS;

'''


            + NOTUS_LOGO + Formatter.LS


    private static final String ENTER_COMMAND_MSG = Formatter.LS

Can move this over to formatter class, and have a static function to get this.

'''suggestion

private static final String NOTUS_LOGO = LS

        + LS

        + "     .-') _               .-') _                 .-')    "

        + LS

        + "    ( OO ) )             (  OO) )               ( OO ).  "

        + LS

        + ",--./ ,--,'  .-'),-----. /     '._ ,--. ,--.   (_)---\\_) "

        + LS

        + "|   \\ |  |\\ ( OO'  .-.  '|'--...__)|  | |  |   /    _ |  "

        + LS

        + "|    \\|  | )/   |  | |  |'--.  .--'|  | | .-') \\  :' '.  "

        + LS

        + "|  .     |/ \\_) |  | |  |   |  |   |  | |( OO ) '..'''.) "

        + LS

        + "|  |\\    |    \\ |  | |  |   |  |   |  | | '-' /.-._)   \\ "

        + LS

        + "|  | \\   |     ''  '-'  '   |  |  ('  '-'(_.-' \\       / "

        + LS

        + "'--'  '--'       '-----'    '--'    '-----'     '-----'  "

        + LS;

'''

The message is from pin command


💡 If no tag color is specified, the color of the tag will be that of the default white color.

Are you moving all the message to a class? If so you can call the format function inside ExitCommand execute so that there is no need for an additional call here

Want to move all the asci art and messages to another class?

I dont think this class need a test since its just messages.

Can this be a function in notebook, say getNote(String noteTitle) and return true/false or the Note object/null. There should not be an ArrayList needed.

Is there a need to get the content first?

Can refactor most of this into inputContent(), including the do while.

Eventually it should just be:

content = inputContent;

if (content == null) {

return Formatter.formatMessage(unsuccessful message);

} else {

note.setContent(content);

...

return Formatter.formatMessage(successful message +...)

}

No need to check if getTagsName is blank. Will do the fix on the formatter side.

Is systemException thrown anywhere in Formatter?

Make the function return the boolean instead of throwing exception? Also, should the equals be equalsIgnoreCase?

No need for this exception since its the same as the COMMAND_UNSUCCESSFUL_MESSAGE in your deleteCommand. I have added my comment on the function to return the boolean instead of throwing exception.

Unused import package

Catch the StringIndexOutOfBoundsException within the function and return null instead.

Unused import package?

There is a notebook.getNote(index) function, can use that instead for simplifcation

Possible to let notebook.deleteNote(index) return a boolean as well. So that can move the isDeleted outside of the if else (titleIsBlank) and do a consolidated return..

i.e.

if (titleIsBlank) {

title = getTitle()

isDeleted = notebook.deleteNote(index)

} else {

isDeleted = notebook.deleteNote(title)

}

if isDeleted

return success else return unsuccessful

Is there a need to new the array list here?

No need to have this variable here. If title is blank, you can reuse the variable "title" and assign the title here. At the end you will only need 1 return Formatter.formatString(ARCHIVE_NOTE_MESSAGE + title);

Why is there a need for archivedNotesList? Since all the note can only have unique names, wont it only return 1 note object? There is a getNote(title) function in notebook, you can use that instead.

getNotes.get(i) can be replaced with getNote(i)

Is there a need to new the arrayList here? Ideally, ListNoteCommand should just call a function in notebook, say getNotes(bool isSorted, arrayList>Tag>) (currently is only getNotes(), you can overload it). All logic handling should be within notebook class itself to sort and return the requested arraylist of note. This is also for reusability, having the function in the notebook instead in this ListNoteCommand Class. Do refer to the word doc I shared.

All formatting should be inside formatter class. Can change the formatNotebook to formatNotes(arrayList Notes)

Similar to my comment for archiveNoteCommand

Unused import statement

new in the constructor

This function shld be split into 2. 1 is getArchivedNotes() where it just return the arrayList. Another function is archiveNote(int index), overloaded with archieveNote(string title). ArchiveNoteCommand will call this function and this function will handle will removing of note from the original list and adding into the archivedNotes arrayList

Likwise there should be an function for unarchiveNote(int index), unarchiveNote(string title)

Discussed that archiveNotes(title) should return a boolean to check if the notebook manage to archive the note successfully

In what situation will this happen?

Same comment as ArchiveNoteCommand

I think @prachi2023 has this line to display the info for pin note command. Can have the function as getPinnedString()

The Boolean notebookToggle can be renamed to isPinned or something similar to better identify its purpose.

As discussed, change the parameter ArrayList>Note> to take in a boolean fromOriginalNoteList or something similar and do a if else statement to determine which arrayList to get the note from. Also, consider the case that the note does not exist, what will the stream return?

No need for this 1 line function.

Handle the case when the noteTitle does not exist.

just do notes.remove(index) here


        notes.remove(index);


        archivedNotes.remove(index);

        notes.add(unarchivedNote);

Similar to archiveNote, handle if title does not exist.

No need to have the formattedString if its just passing an empty string into the function.

can remove the last parameter for the function. Suggested way:

formattedString = getNoteString(pinnedHeader, pinned);

formattedString = formattedString.concat(getNoteString(unpinnedHeader, unpinned)

Change this to a local variable rather than pass in by parameter

Why do you use LS? Wont the output have 2 empty lines? and the sides will be broken? i.e.

|| Title ....... ||

>> LS here>>

|| Content...||

>> LS here>>

=========

Looks clean now!

Realise that this + note.getTagsName can be added when this function is called. i.e. in the command execute

return formatNote(header + note.getTitle + note.getTagsName(), note)

There is a formatEvent function in Formatter, use that to format an event object instead of calling event.toStringArray(). Overload it if necessary.

Expected code:

timetable.addEvent(event);

return Formatter.formatEvent(COMMAND_SUCCESSFUL_MESSAGE, event), where formatEvent takes in a header and an event.

There is no need for this function here, handle this in Formatter.

Handles in formatter.

This should be handled in formatter instead of splitting the content here. There is an empty function called formatEvent. Overload if necessary

Suggested: formatEvent(event)

encloseRow("Event: " + event.getTitle();

encloseRow("Date: " + event.date.toString() + "\tTime: " + event.time.toString());

encloseRow("Reminder: " + event.isToRemind);

encloseRow("Repeating: " + isRecurring);

As per previous, format in formatter. Can check if the event is an instance of RecurringEvent and enclose the necessary info.

As per other comments.

Can call formatter to get the expected output, do not have to manually type out. In the event that the format changes, you do not have to edit the format here.

expected = Formatter.formatEvent(SUCCESSFUL_MESSAGE, event)

Maybe can overload another formatEvent to take in Header as one of the parameter. That way, just have to call 1 format function.

Likwise, same as above

Do the enclosing here and just make it return the formatted string straightaway. Can overload it to take in a header for printing 1 event.

Likewise, returns the formatted string

Noted on this.

Noted.


        3 MD


        2 MD


        Tag Notes

        ----

        5 MD

    ]


        5 MD


        2 MD


        2 MD


        2 MD


        2 MD

Why did you remove this variable? I thought it is needed to differentiate the notes when saving to txt file? Or is the archived note list saved in a separate txt file?

Ya, I am referring to loading from text file, what differentiates them to be loaded into respective array list?

I have went through her code, the boolean is still needed. When loading from text file, it uses Parser to create a new AddNoteCommand, inside Notebook the addNote(note) function can have an if else, based on the boolean isArchive and then add into the respective arrayList.

Is the boolean added back in? Looks like it's still removed.

You may want to have an arraylist of note for the expected output and then use the Formatter.formatNotes. In the event that the format changes, you do not have to manually change the expected string again.

All expected string can use the Formatter functions, for this case encloseString(title). Since the purpose of the command test are to compare the result and not the formatting, it is ok to just use those functions.

Likewise

He did that in the execute return, since this is a private static final, the time period cannot be added here.

This should be formatEvents or formatTimetable. See suggestion for formatEvent(event) in the #108

These functions can just sort and compare the events and eventually just return an arrayList of event.

Possible to change the parameter such that it takes in an arrayList of events instead of HashMap>Month, HashMap>Integer, ArrayList>Event>>> timetable?

In terms of reusability:

for (Event event: events) {

formattedString = formattedString.concat(formatEvent(event));

}

There is not much difference, I use concat mainly because intellij will give warning for using += in loop, so I use concat for all cases.

If thats the case, I think can leave it. The only concern is that the data struct looks complicated.

Why does loadAll function takes in storageManager as one of the parameter?

Why is there a getNoteContent here since it is adding a new note? I believe the line below that @Nazryl added is to pass his JUnit test.

Would there be any difference if saveNote is not a static function? Since all command classes contains a storageManager object, all are able to access this function.

Can use Formatter.LS if this is a line separator.

I see the reason you pass in storageManager is only for setData. Can replace it with 'this' so that this function does not need the StorageManager parameter.


            command.setData(notebook, timetable, tagManager, this);

@R-Ramana The boolean for isArchive is still needed for loading.

Currently we have 2 arraylist, 1 for all the notes and 1 for archived notes. Need another loop for the getArchivedNotes()

@R-Ramana Here is the saving part.

This is a temporary suggestion I provided her. Attritbute.BRIGHT_COLOR_TEXT().toString cannot be used as case expression because they are not constant. There is another way to use switch, but since its related to color, I will edit on my side.

What error? I tried and it works.

Can declare this as a const?

Can remove it since its not used anymore right?


            formattedString = formattedString.concat(encloseRow(EMPTY_SPACE + truncatedContent));


                        int offsetSet = stringBeforeReset.indexOf(POSTFIX);

This 2 can keep as well

If there are no reminders, it will add a none, then would the output be: "None reminder"? Sounds a little weird.

Why is there 2 notes of the same title? This shld not be accepted when adding note.

Is this function called in other methods? Changing this boolean may affect the output of others. Also, from the parameter name, it sound logical that if it is not isArchive, it will search through the notes arrayList as described in the JavaDoc comment.

Suggested change should be inside the archiveNotes(String noteTitle) under notebook.

Note archivedNote = getNote(noteTitle, true); ----> Note toBeArchiveNote= getNote(noteTitle, false);

yup it should! I just generate a template test class therefore it consists of the execute function inside. All the test functions should be renamed accordingly.

The intellji gave warning saying that the field may be final so I changed it. Will be reverting that since the int cannot be final. Likewise for the rest of the similar cases below

For this, because the user can choose not to input the sort parameter, therefore the Boolean can hold a null value, which is used to check later

Don't really think there is a need for a function because this help string will be a constant. Can actually merge with the declaration of the HELP_STRING. @R-Ramana your view?

My intent is sort by usage. Noted on the separation.

No worries

This one is the ide auto ident to next line.

This is for tag-e

This is deleting of the tag and not untagging. Will update the content.

Noted. Will edit it

Currently adding of note doesn't check if the title match any archive notes. So I added this. Refer to my changes in AddNoteCommand

Failed CheckStyleTest

Have you pulled the latest version? There seems to be conflict between the versions.

Yup I will close it.

Btw can we add the .log files into gitignore. Everyone can generate their .log file on their own local copy.

Trying another fix for this

I will leave this open until until I find a better fix.

Will not close previously linked issue.

This is not a bug. Intellji prints white font as black and vice versa. See attached image to change it such that it displays the correct color on intellji

Yup!

Yup yup

Implements #98

Closed by #136 and #117

@Nazryl

Suggested fix: remove the index-- after handling the PREFIX_INDEX because there is another check for the index value after the for loop. The check is needed in case the user did not enter "/i ...."

return new EditNoteCommand(index - 1, note, addLists, editLists, deleteLists, isInput);

@prachi2023 The step to reproduce the bug is wrong, it should be changing the title manually in notebook.txt. Thus, when loading, it attempts to read an non-existent txt file and the content is empty. Maybe can do an error handling. When loading, if it is unable to find the note txt file, skip it. Also do a save all at the exit of Notus.

@prachi2023 Can change the message to be "title" pinned/unpinned.

@R-Ramana likewise for archiveNote,

unarchiveNotes function in notebook, the getNote(noteTitle, false) should be getNote(noteTitle, true)

Unable to reproduce bug, more test required. Possible scenario is that there exist a "JavaDocs" note that is archived therefore list-n does not show that note. May have to modify the 1st error message to indicate that the note is archived or unarchived.

User entered wrong command, should be "tag-n" instead of "tag"

Requested clarification from user.

See #168

Suggest to remove the screenshot?

Update UG

Could be terminal issue.

Unable to reproduce bug. Requested additional info from the user.

#168

https://github.com/0xZ3RO/ped/issues/2#issuecomment-719929063

If it is a test method should we not follow the following naming conventions?

whatIsBeingTested_descriptionOfTestInputs_expectedOutcome

testMethodName

Maybe can update this to COMMAND_USAGE to be clearer on what this String is for 😃


    private static final String DELIMITER = "|";


    public int getSize() {


                            + DELIMITER


                            + DELIMITER


        return notes.get(index);

Thanks for updating on my behalf 😃

As mentioned above


        private final String COLOR;

Private final right, so should be all CAPS? Can just commit directly from my suggestion 😄


    private final int INDEX;

Same as above


    private final ArrayList<Tag> TAGS;

Thought I made the same changes in my previous PR haha

Should it be TAGMAP?


    private final Map<Tag, ArrayList<Note>> TAGMAP;

Just a minor indentation issue. For all my suggestions, if you are okay with it, feel free to commit directly, using the "commit suggestion"! 😃


    public static final String COMMAND_WORD = "add-n";


     * @param note refers to the note to be added.

Can we add some 1 line comments to explain what the if-else block is doing?

I don't see the System Exception being thrown in the method?

Remind the person who is working on AddEvent not to forget this part! 👍


     * @param userMessage Original string user inputs.

     * @throws SystemException if an error occurs.


     * @throws StringIndexOutOfBoundsException if an error occurs.

     * @return A string of converted content input


     * @throws SystemException if an error occurs.

     * @return Result of the delete note command

Same as before, please remind the person for AddEventCommandTest

Good call!

This part is not yet completed?

Made the code more concise. Feel free to commit the suggestion directly


                + "the index that is indicated when you print the event list"),

Don't think the delimiter is necessary


        String dateString = "Date: " + date.toString() + "Time: " + time.toString();

Use InterfaceManager.LS to standardize for new lines.


        return titleString + InterfaceManager.LS 

            + dateString + InterfaceManager.LS 

            + remindString + InterfaceManager.LS

            + repeatingString;

Added a new line




     * @param input User input to be parsed

InterfaceManager.LS to standardize new lines.


        return "Added the following!" + InterfaceManager.LS

            + InterfaceManager.LS + event.toString();

Same as above.


    public static final String COMMAND_SUCCESSFUL_MESSAGE = "Event deleted:" + InterfaceManager.LS;

Can include the system exception also


     * @param index Index to be removed.

     * @throws SystemException

Added a line break


     * Gets the timetable for a specified month and year. Includes multiple recurrent events.

Same as above + phrasing error.


     * Given an ArrayList of events, get all events that will occur during the specified time period.

Added line break


import seedu.duke.command.ViewNoteCommand;


import seedu.duke.data.timetable.DailyEvent;

import seedu.duke.data.timetable.Event;

import seedu.duke.data.timetable.MonthlyEvent;

import seedu.duke.data.timetable.RecurringEvent;

import seedu.duke.data.timetable.WeeklyEvent;

import seedu.duke.data.timetable.YearlyEvent;



import java.time.LocalDateTime;

Added a line break


     * string trimmed.

Same as above


     * for it to be recurring and to set reminders of the event.

Maybe can add this in the JavaDoc

Added Line break




     * @param userMessage User Input without the action word

Fair enough!

Just minor phrasing


     * Provides a description of how the command should be used.

     *

     * @return a string description of how the command should be used.

Removed extra line

If final I think it should be in caps


    private final LocalDate END_RECURRENCE;

    private final String RECURRENCE_TYPE;

endRecurrence should be all caps right if this is for the final? Think got a few files to amend in that case

Is the throws SystemException necessary here? I don't see it being thrown.


    public void deleteNote(int note) {

Remove extra line

Should we throw the SystemException here?


    private Command prepareViewNote(String userMessage) throws SystemException {

Think maybe we can have this at the very beginning?


    private static final String WELCOME_MSG_STRING = "Welcome to NotUS! Type \"help\" if you need to see a list of commands and their usages.";

I feel this breaks the flow.


    private static final String COMMAND_UNSUCCESSFUL_MESSAGE = "There are no matching notes. "

            + "Please try another search query.";

@brandonywl Yup, so when I was using Boolean it was because there are 3 cases. Default (which is chronological order), ascending or descending. Hence I wanted to use Boolean which can hold true, false and null! boolean on the other hand cannot hold the null value.

Should we sort this by alphabetical order? I mean not a big issue, but if needed, I can do it in my next push? Or if you prefer to sort by usage, then maybe have a line break between each use group?

If we are splitting by space, should we use a whitespace regex?


        String[] tagInfo = tagsInfo.split("\\s+", 2);


                    title = checkBlank(infoDetails[1], exception);

To standardize with the rest of the new lines


        String stringToPrint = note.getTitle() + InterfaceManager.LS;

Same as above


            stringToPrint += tag.toString() + InterfaceManager.LS;

To standardize with your other PR


    private static final String INDEX_OUT_OF_RANGE_MESSAGE = "Index is out of range.";

Same as above

Think you can make this statement clearer, and missing param

Add @param for the javaDoc?

Would this be possible?


                result += InterfaceManager.LS.repeat(2);

I feel like there is arrow-head coding here. Would this work?


        if (reminders.size() == 0) {

            return result += "No reminders today!";

        }

        

        boolean first = true;

        for (Reminder reminder : reminders) {

            if (!first) {

                result += InterfaceManager.LS;

            }

            first = false;

            result += reminder.toString();

        }


        EXCEPTION_INVALID_LIST_TIMING_FORMAT("Your input list timing query has a wrong format. "


        EXCEPTION_INVALID_TIMING("Your input has a wrong format for the date time input. "


        EXCEPTION_INVALID_REMINDER_FORMAT("Your input has a wrong format for the time before reminder. "


        EXCEPTION_INVALID_RECURRING_TYPE("There are only the following recurring types: "


        EXCEPTION_INVALID_TIME_UNIT("That time unit is not accepted!"),

Do we assert here? I think assert at a Test Class right?

Whitespace


     * 

     * @param title Title of event.

Added whitespace


        String dateString = "Date: " + date.toString() + "\t Time: " + time.toString();

Again looks like arrow-head coding. Can we try this?


        }

            

        comp = time.compareTo(o.time);

        if (comp != 0) {

            return comp;

        } else {

            return title.compareTo(o.title);

        }


     * @param dateToRemind Date of reminder

Seems like you are not using the userMessage


                return prepareRemind();

Actually if you have nothing to prepare why don't you return this directly at the parser switch case above?


    private Command prepareRemind() {

        return new RemindCommand();

Added Whitespace


     *

     * @return Instantiated Timetable with 1 event.

Added Whitespace


     *

     * @return Instantiated Timetable with 4 default events.

Added Whitespace


     * Wrapper around assertEquals for code readbility.

     *

For more clarity


     * @param tagName Name of the tag to be set.

     * @param tagColor Color of the tag to be set.

Feels like a little bit of arrowhead coding. Jun Xiang has a good suggestion to reduce it.

noOfChar feels a little hard on the eye. Can I suggest something else?


    public static void deleteLine(StringBuilder commandInput, String characters, int charCount) {


     * @param charCount Number of character. 0 to remove new line, 1 to resume typing on the same line.

Easier to make changes if needed - using markdown syntax


**Prerequisites:**

* JDK 11

* IntelliJ IDE


Import the project as a **Gradle project**.


**NotUS** manages the flow of the application. On launch, it will create the necessary components, as listed above


**Parser**


1. Receives the user input message as a whole.

1. Interprets the type of command and splits the message to identify all the parameters provided.

1. Creates and returns the Command class respectively.

 

 {insert Parser.puml here}



**Command** classes

All of our Javadocs was @param parameter definition. There was no additional whitespace to align the columns. If we are doing this, should we then standardize for all JavaDocs? Would it be easier to just have a single space?

Same as in the previous PR, can consider concat for standardization

Same as the previous PR, can consider the following


        result.set(result.size()--, result.get(lastIndex) + endRecurrenceString);

I think this can come under the last subsection under section 2

Don't think the PinCommand is created by the parser function right? Maybe can phrase it better?

1. Toggles the pinned status of the specified note.

1. Returns the title as well as the pinned status of the note.

Yeap I guess! But this one can leave out for now. Jun Xiang will be modifying the Parser command to make our code reflect more OOP

So when we used String in the formatter class, we have been using .concat(). Might want to consider doing these += as concats (in the Formatter class)

If you are not using the lastIndex anywhere else maybe can consider


        result.set(result.size() - 1, result.get(lastIndex) + endRecurrenceString);

Maybe can add the time period parameter?

Not much as Jun Xiang mentioned, but I think it would be better if we can standardize the code 👍

Would you be able to create a StorageManager storageManager variable and access storageManager.saveNote(note)?

Put "unable to write file" as a COMMAND_UNSUCCESSFUL type variable. Then pass that variable instead


        EXCEPTION_FILE_CREATION_ERROR("Unable to create the file!"),

        EXCEPTION_FILE_NOT_FOUND_ERROR("Unable to find the file!"),

        EXCEPTION_FILE_DELETION_ERROR("Unable to delete the file");

@prachi2023 FYI, have re-added it in the Note class. Please take a look if you need to use.

Can we use a switch statement?

INDEX STRING is not very clear on what it does. Do you think we can use something like Line number or something?

As mentioned previously, I don't think the other JavaDoc comments had white space. Maybe we should standardize?

Not 100% sure if a user will understand what exactly Missing line content is. maybe can phrase it better?

As above for the whitespace


//@@author prachi2023


//@@author prachi2023


The ParserManager manages the creation of specific parser objects based on the type of command. The parser then makes sense of the user input and calls the respective commands into action.


1. Interprets the type of command and creates the respective parser for each command.


1. `ParserManager`: Creates a suitable parser, based on the command, to make sense of user message. The respective parsers then make sense of the information and calls the respective commands.


💡 Note that the alternate paths in the sequence diagram above are not exhaustive. There is an alternate path for each unique command. As there are too many paths, they are omitted from the diagram. The Command objects in the diagram are used to represent a generic Command object that is created through the Parser. Refer to the next figure for more details.


Notes and Events inherit from the abstract class, TaggableObject, and TagManager contains a map of individual unique tags to an ArrayList of TaggableObjects. The TagManager handles the creation, deletion as well as the tagging and untagging of tags from notes or events.


### <a id="tag-n"><ins>3.13 Tag/Untag Notes:</ins> `tag-n`</a>


💡 If the user tries to tag a non-existing tag to a note, the application will automatically create a new tag, and add the tag to the note.


`tag-n /i 1 /tag CS2113`



`tag-n /i 1 /tag CS2113 /tag important`


### <a id="tag-e"><ins>3.14 Tag/Untag Events:</ins> `tag-e`</a>


💡 If the user tries to tag a non-existing tag to a note, the application will automatically create a new tag, and add the tag to the note.


`tag-e /i 1 /tag CS2113`



`tag-e /i 1 /tag CS2113 /tag important`

Only tags? Can we do it for events as well?

My apologies, I copy-pasted from the change I made for tag-n then forgot to update it.

Since this is not part of the code and people refer to the diagram, not so much of this code, so I think can remove all the whitespace (extra lines) since it is inside the enum.


💡 Each note has to have a **UNIQUE** title and it is **CASE-INSENSITIVE**.

Not sure if the line break is necessary, don't think some of it has? Should probably standardize

What is this used for?

Yea I realized but I forgot to remove this comment! But yeap thanks!

Addressed 👍

Will not print, will return a String instead

Because I fear if we do just "TAG" the person might assume it is only for 1 tag. What I can do is PREFIX_TAG + " TAG " + PREFIX_TAG + " TAG "... to show that they can add multiple tags? Would that be better?

addressed!

Yup, good point! Converted

Okay, made the changes will push

Yup doesn't work. So will not be making this change

You are right, my bad! Thanks

Could've just returned the string actually

Already in the code

Will do so with my next PR!

Good idea. Will do so (referring to both of your suggestions)!

getNotetitle() returned a boolean, I have changed it but I have decided to overload it.

This function isn't for me/nor did I create it. Used in DeleteNoteCommand

In that case, this function would actually be redundant, could just call the getNoteString directly

Discussion taken off GitHub


        3 MD


        3 MD

No need to check for is archived in my implementation. The note will either be in the archived notebook or in the main notebook. When archiving/unarchiving, the note will be removed and added to the respective notebooks.

How is the files being loaded currently? It traverses notes (from notebook) right? Can just make it traverse both notes and archivedNotes?

Will add it back in that case

Updated, requesting for a final review prior to merging

Could have but I just keep it, in case we need it. Can remove it during the code cleanup if confirm not needed

A pull request was merged addressing this issue, but did not link the issue to the PR correctly. Hence a need for manual closing of the issue.

I realized that the test methods did not follow the coding standards. Just make the changes in your next PR 😃

Created PERT and Gantt charts.

Btw can we add the .log files into gitignore. Everyone can generate their .log file on their own local copy.

Can. will make the changes in my next commit.

Closed as completed with PR #102

Its fixed on my side. I havent commited.

Noted! Feel free to close the issue when you make the PR.

Greater clarity included in the User Guide to address this issue.

LGTM, well done!

Thanks for helping me to remove this, forget to do it on my side!

Hey, I think we might as well just keep the interfaces for easy reference to the behaviors

Looks good! Might be good to add some JUnit tests for this class now even if they just assert true for now, just to keep in mind our desired behavior for each method

Good use of packages

Good catch, forgot to include this.

I think this one may have to be "options" as it is a collection. When we call an instance of the OptionList we can probably call that optionList

Good idea but may be redundant as we can call "length()" on the ArrayList

Thanks for adding this for me

Thanks for changing this LGTM

Thanks for moving this for me

Maybe we can move this into the "quiz" case next time so we don't check for the command twice

Remember to initialize this next time as well

sorry I mean to comment on ui, didn't know the top part got included

Thanks for adding this but would be good to use a separate branch next time

Good idea to separate these responsibilities to two different classes

I see what you are trying to do here but is there a reason for this class to be different from the other displayables and do we need this extra method?

I think you should try not to depend on this class in to create the QuizQuestionList object as this violates SRP

Why does this not implement DisplayableList, maybe can change the way we think of and name this class?

Good update! Did you managed to test the other commands with IO testing also?

Looks good!

Will try to abstract this into another class, thanks for implementing with this for now

You can load from the test storage data/test/example.json if you do not want to create the topic for this test

Thanks for changing this, forgot to account for wrong commands

Good use of assertions

Good SRP

Good idea maybe we can add this to the other DisplayableList classes

Sorry need this to be one word something like Java_Basics for the parser

Thanks for changing this

Cool idea to add the number of questions

Nice, good to check our exceptions

Good use of assertions

Useful warning log, great!

Good idea to log this but maybe using constants can help since most of the words in these strings are the same. Will open an issue to use more OOP for v2

good idea to protect this

good slap

Good placeholder

Thanks for changing this

Good use of the interface

Nice description of the implementation for the parsing

May have to standardise our headers later

Good work on this class diagram

good work on this sequence diagram

I think the first one is repeated now because I added it previously but thanks for adding all of the rest

You can take out add, delete and the empty constructor as I think @joelngyx is working on removing them from our code

This header isn't showing up properly I think you need to add a space after the # , noticed this in a few other places as well

I think you are missing the interface label for DisplayableList and Displayable. Some of the arrows are also out of place and arrowheads missing so you might want to check the diagram again.

Might want to split this as it may be confusing. Also not sure if you should be returning control to each class instead?

nice javadocs

Nice solution to allowing the number of questions and topic name to be swapped.

Thanks for putting this placeholder, will try to get it done soon

Nice formatting

Good idea to make the usage clearer

I think this is ok for now, i will add in a storage system to load the bookmarks along with notes

this one as well may have to create a User class to include notes next time but for now it seems fine

I think for this you can call execute on the command and then just assertTrue(true) which represents that the the execute doesn't enter a loop since a bad topic was given

same as above

Nice! Thanks for changing this

Good idea to abstract this

I know this is a draft but you should probably be taking the TopicList in the constructor

Nice, good implementation of this feature

Looks good but there seems to be a random * in the top right area of the diagram

Nice diagram

Good explanation of the implementation

Good use of visual aids

Thanks for rewording this, alot clearer

Well done updating the UserStorage to match your Note changes

Nice use of constants

Good update to the user guide, not sure if these sections should be nested under a notes feature

Good idea to let users choose their timing but I think you need to update this message to match and change the input file to start up the quiz correctly

good idea to make this new command for timer timeout

Good idea, will have to change userstorage a bit to accomodate this as well

looks good, nice formatting

Nice, makes it alot clearer

Nice improvement to code coverage

Good addition to the junits

Good use of javadocs to make the method purpose clear

Nice addition to the code coverage

Good change

Nice javadocs

Good catch about the timer

I think the method calls a bit wrong as you should point to the object that is called. Liked question.getInnerList() should call on a question object and getInnerList() should call on a questionList object

cool change, no need to update this everytime anymore

would be good to add some screenshots here

Done thanks

Yes sorry thanks, need to get out tests up quickly to catch these mistakes

Sorry I implemented this in #13 as it will load and parse the JSON from the file before passing everything as our own objects

Did you finish this in your pull request #34 or are there still more methods you are implementing? Remember to add your Junit tests also

Was this done in your pull request #22 ?

Need to make sure it works with a hintCommand

Good idea to maintain SRP

Done in #53

Done in #59

Done in #71

remember to update storage and the json

Forget this, dw do alr

For easy reference to first version:

Class diagram:

Sequence diagram:

remove from functionalities

remove functionality

Saw that removing these methods disrupts some Junit tests, you can either construct using an arraylist for those tests or load from the test json. If this takes too long work on the ugdg first and we can get this up a bit later!

I think you should move the exceptions higher up so it doesn't reach this assertion

Done in #126

Decided to keep IO testing as backup

Being done as UG is updated

done in #136

closed by #121

same as #176

for this one maybe must make it clear that once u use a hint u can never score 100%?

same as #189

same as #188

How did they select 6 questions for this? @Zhi-You

Consider removing this.

public String toString() {

public Resistor(double value) {

Consider changing this to Resistor object


package seedu.duke.component;



public class PowerSource {

    private static final String SI_UNIT = "V";



    public PowerSource(double value) {

        super(value);

    }



    /**

     * Returns the value and unit of PowerSource object.

     *

     * @return String of Power Source value

     */

    @Override

    public String toString() {

        return (value) + SI_UNIT;

    }

}

The lack of a newline is causing checkStyle error

Perhaps the name otherValue is a little less intuitive? Consider naming it valueToBeAdded or something along those lines.

Unsure of the purpose of the ui object in this function

I see. Okay it's alright then

Ohh okie alright gotcha

Yup was thinking of setters, wasn't sure whether I should add. Will add after tdys meeting!

While I know having break statements out here is kind of useless but does this violate coding standard? https://se-education.org/guides/conventions/java/basic.html#layout

Was going to ask about if the current template wasn't set, but saw this. This makes sense!

Oh wow that's...interesting

Quite interesting that you added this to gradle properties to be able to render Ω, μ. Hadn't thought of this before!

Consider changing the array name to a plural like templates? https://se-education.org/guides/conventions/java/basic.html#naming


        String[] templates = {"r", "rl", "rc", "lc"};

If you commit the previous suggestion you'll have to change this too:


        for (String s : templates) {

Same with this collection of values:


        String[] effValues = {"reff", "leff", "current", "power"};


        for (String s : effValues) {

Nice

Great description!

Interesting!

Any particular reason as to why you declared these as final? Because when a variable is declared with final keyword, its value can’t be modified, essentially, a constant.

Here as well.

Yup it will work, but the point of declaring it as final is sort of nullified

The test for this method in the RcTemplate_Test will not run because it tries to catch DukeException but the assert keyword throws a different kind of execption.

Might consider removing this, since it is already tested by JUnit?

Fair, then might have to make the names capitalized right

Nice! Might have to add rigorous tests for this later on.

Nice

Good use of generics!

Seems to be an unresolved merge conflict in this file. Might want to edit this file.

This is already tested in a JUnit test under AddCommand test, hence will cause a build gradle error. Consider removing this.

Not sure if tested through JUnit, but if build gradle fails checks again after removing the other statement, then this is likely causing the issue too.

Any particular reason for including so many blank newlines haahaha

Same with this statement. I think avoid asserting in this function (even in the other classes) since it is kind of already taken care of by exception handling. The program will terminate in case the user enters an incorrect component

Update: Consider removing this

Hmm consider naming it SPACES? Since we could change the number of spaces if required later on without having the name become obsolete

Interesting, what does this dependency do?

:0 gotta remove lah

Will have to change the gates to this kind of structure

Nice!

What is this file for?

We could assert that the logic will not call anything out of bounds?

Yup, sounds good!

Good eye

What is xand? Isn't it called xnor..? (・–・;)ゞ

Idk if this matters but there's a typo in sequence

Ohh lol I see

The CLIrcuit Assistant thing, I had removed the logo because of runtest. However, forgot to put it back..

Err I'm not sure but why is this summary table in TutorialCircuitCommand?

Can use the summary table from SummaryCommand, perhaps create an object of SummaryCommand and print it?

Wait.. this will print once the person goes through 8 commands, and then print again because 8 > 6 commands done

Perhaps could have added Current Voltage printed after the template?

I don't think we need to instantiate anymore since we are inheriting SummaryCommand here

Can remove this also

BTW should we add this before the binarytree part? Because I've referenced Gates and all in that section

bad

So many spaces :0

However, such an approach allows users to add...

I'm not quite sure whether this would be an issue since we could constrain the user from adding certain components to the circuit. Perhaps you could say that a Linked List would not be able to simulate a proper closed circuit-like system, therefore was not chosen...?

Lol the random π at the end

Also, since we talk about adjacency matrix here, consider calling the implementation "Graph with Adj Matrix" or something since an adj matrix is a way to rep a graph.

Also will have to add the term Adjacency Matrix to the glossary.

Yup! Good idea

Okay will do!

Yup I'll add it

Noted! Will add in all getters.

So, remove the else and break; in the if block? Will break work for a method to return to call.. or should I just empty return;?

Yup noted. I didn't see that before.

I see. Yup, that makes sense!


import static org.junit.jupiter.api.Assertions.assertEquals;

import static org.junit.jupiter.api.Assertions.assertThrows;

import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;


        CalculateCommand c = new CalculateCommand(LC_TEMPLATE, CAPACITANCE);

        assertDoesNotThrow(() -> c.execute(UI));


       assertDoesNotThrow(() -> c.execute(UI));


        CalculateCommand c = new CalculateCommand(LC_TEMPLATE, CAPACITANCE);

        assertDoesNotThrow(() -> c.execute(UI));

Good idea, will do

Yup alright

Will try, was getting errors with the fancy fonts (tutorial mode) and table for command summary

Then this thread will terminate execution and jump out of the help mode. ^_'

Okay fair, I'll move it

Oops I had initially made it byte then changed everything else to int. Seemed to have missed this one out. Thanks for pointing out!

Actually, the list is initally populated with 16 null values, so technically we gotta check for if the 1st value is null. Might have to change the isEmpty() function.

Noted. So I add set output as a separate fn right?

Good idea, will do.

Hmm, makes sense

Oh no oops, perhaps I'll add it in a next PR?

Accidently got added cuz intellij optimised the imports lol

Yup yup had mentioned when i created the PR:

Have to update runtest.bat with apt commands.

Lol didn't notice this. Good eye

Couldn't think of a good way to generalise the tutorial command whilst also splitting between the two kinds of circuits. Since the both the classes already inherited from BooleanCommand/CircuitCommand I thought the only way we could define the characteristics of a TutorialCommand and implement was through an interface (oof)

@hughjazzman I might be able to add a function declaration here (startTutorial()), I guess I add it in another PR, or ?

Okie sure

Oof sozz should've informed you

You're right! And I was thinking the same, just kept it in case it would be used somewhere else...should I remove it?

Okay yep

Good idea! Will do that.

Noted

I see, however since this is recursive, the Gate object will continuously be created again and again right...not sure if that's entirely a bad thing

Okay I'll change and see

okay noted

OH NO i put it after lmao

okie

wdym?

we need to check the circuit tutorial right

fml

Perhaps JavaDoc to be added, but other than that LGTM.

Ohh right I forgot. I'll do it.

Ok will add this

Finally -_-

Okay sounds good!

OHMYGOSH IT FINALLY PASSED

Task Assignment:

Package | Person

-- | --

Command | @sevenseasofbri HelpCommand, CalculateCommand, AddCommand, SummaryCommand

Command | @dorianfong98 ExitCommand, SetCommand, Command, TemplateCommand

Component | @oasisbeatle

Parser | @hughjazzman

Template | @PraveenElango

LGTM!

LGTM, will need JavaDoc and maybe a comment showing the format of the printTree?

Yup can, I'll do that

Just realised the name printTree is sort of misleading since the function really just modifies the 'tree' variable and really doesn't print the tree. Will change the name.

Sexy UML diagrams.

LMAO DHSFBJEFKERF

BTW I'll add the glossary after everyone has added their parts to the dev guide!

Will be done through CalculateCommand

With HelpCommandParser

Can @oasisbeatle or @dorianfong98 do a short write-up for CircuitTemplate and @sevenseasofbri or @dorianfong98 do one for BinaryTree?

Okie I'll do it

Also you gotta update expected and unix expected files so that the runtests pass

Yo I'll add this soon before class perhaps

CheckList

Add Introduction to concepts.
Add label for images.
Explain bullet points used at each command.
Add numberings for section headers.
Add clear instructional steps for each command.
Add diagrams/formatting/colours/arrows/tips for each command.
Add glossary (optional).

LGTM, should we use the {\:toc} mentioned here: nus-cs2113-AY2021S1/forum#108 (comment)

Yeah I think we should try it out. I'll try add it now.

Lol I don't think I'm doing this properly

Hi @nishanthelango thanks for the review! 😄 About this diagram:

Yup it's supposed to be a solid line, thanks for pointing out! I just wanted to ask if this notation is ok for object creation in sequence diagrams, because I can't seem to find a way to do it the way it's been specified in the textbook when I use PlantUML.

(Update: I'll still look around though, I'm kinda new to PUML lol)

@nishanthelango OHH thank you so much! (I should've looked at the docs better oof) 😄

Yo we gotta do this too I forgot abt this

@PraveenElango could you add in printing the voltage + circuit since you worked on UI last iter

There is no series parallel V - must put in UG documentation. As for L- i think it is working fine. @hughjazzman you're doing add command for the UG so probably mention that

GUI Constraint gotta mention that the simplified circuit is shown in the UG. @AY2021S1-CS2113T-W13-3/developers

Must be clearer in the Docs I suppose @AY2021S1-CS2113T-W13-3/developers

Not a functionality bug. But must specify what can be added to a template in the UG. @hughjazzman @dorianfong98 since you guys are doing add and template command respectively

UG error:

@hughjazzman u taking care of add component so perhaps change this

Update instructions on how to use tutorial command, explain how it will remain in a loop unless you decide to exit. @oasisbeatle

Summary command is literally the 1st thing in the UG. But might have to include diagrams I suppose. @AY2021S1-CS2113T-W13-3/developers

Need to remove help from quickstart and other places:

@AY2021S1-CS2113T-W13-3/developers

No additional functionalities will be added post v2.0 right. @AY2021S1-CS2113T-W13-3/developers

@oasisbeatle we need to make the UI for the tutorial commands more intuitive. And also add that to the documentation.

Need to improve UG section on tutorial command. @oasisbeatle

Yo this is a good suggestion. @hughjazzman might wanna change it to this?

Don't think this should be too much of an issue? Because the template command needn't care about the parts after. @AY2021S1-CS2113T-W13-3/developers

No features to be added after v2.0. Man, what's the deal with people wanting to store circuits lol. @AY2021S1-CS2113T-W13-3/developers

Ok gotta improve the tutorial commands @oasisbeatle and I

Improve tutorial to produce apt error messages @oasisbeatle and I

Summary diagram can have better examples rather than parameter table and command table @AY2021S1-CS2113T-W13-3/developers

Need to document the templates better @dorianfong98 since you doing template command for UG

Err might have to make the tutorial instruction clear.

Lol gotta add a space at the top @AY2021S1-CS2113T-W13-3/developers

Right same as #136

UG had mentioned this right? Might have to update the add command to show relevant error @hughjazzman

Customise exceptions for our respective commands @AY2021S1-CS2113T-W13-3/developers

This is a bit concerning. Might have to add a check @hughjazzman

#148

This is concerning and I will look into this.

This is concerning, I'll look into this

@hughjazzman agreed

Perhaps you could refer to the MESSAGE_USAGE in ReviseCommand?

Could perhaps separate these into its respective command files like what was done for revise.

Would it be better to include "edit" inside the method name? (e.g. showCardContentBeforeEdit) as it is only for the edit command

Perhaps the MESSAGE_USAGE could be included after the error message?

Not necessary to specify date when adding cards

Date parameter is not needed as it will be initialised in Card class

Not necessary to specify date.

Not necessary to specify date

Perhaps this is not needed?

Maybe you could add a separate function which is similar to this and call it for saving of tasks into file.

Perhaps could also include previous interval too when loading from file

Perhaps MESSAGE_USAGE could be removed as the command is correct but chapter is not found?

Would it be better to have a general method for the chapter and module each, then pass the message to that method?

Should this be done in Storage?

Should this be done in storage?

Need this to be int for assertion

Need this for assertion

Perhaps could also add MESSAGE_USAGE like other commands?

Perhaps could use ui.showToUser as well. And could maybe change to "Command entered: " instead? As some commands may be invalid.

Perhaps could close scanner here?

Perhaps could use ui.showToUser or create a method specifically for printing these in Ui?

Perhaps "Lists the revision completed in ..." would be better?

Perhaps you could include a message for the exception?

Could include error message for exception here

Perhaps you could pass ui from Kaji to this method and call ui.showToUser and ui.showError instead of using System.out.println?

Maybe could refer to the constants EASY/MEDIUM/HARD instead of using integers for the switch case?

Perhaps could change this into one message and denote easy/medium/hard with %1$s and pass the type of difficulty into string format

Perhaps could use "Shows" to standardise with the message usage of other commands

May consider changing this to toLowerCase and use the constants instead of string literals for switch case or changing the constants to uppercase.

Could change this to "Rates" to standardise with other commands

Is it possible to throw exception or return this message to execute so that System.out.println is not called at other places other than execute function?

Perhaps could return this to execute and do ui.showToUser there

Is there a reason why "this" needs to be used? Or setRating is sufficient?

Perhaps return rating would be sufficient?

Perhaps could use caps for parameters

Perhaps could refactor these into constants?

Could include message usage here also

Could include message usage here

is it possible to return the thing you want to print to the execute method and call ui from there? For the methods below as well.

Could change to use of out.print

Perhaps could refactor these as constants?

Maybe can use Messages.CHAPTER instead?

Would this not be needed since it is already checked in parser?

Would this not be needed since it is already checked in the parser?

Would this not be needed since it is already checked in parser?

Perhaps this may not be needed?

Perhaps can just use the COMMAND_WORD from ListCommand

Perhaps can change flashcards to %ss (extra s for the word) in ListCommand and when you use this message, Stringformat with the type Messages.CARD

Perhaps can change chapters to %ss in ListCommand and when you use this message, Stringformat with the type Messages.CHAPTER

if the list command is already separated out, are these two methods still needed?

Already implemented

Is there a reason why a new scanner is being declared every time a command is taken in? I believe this works the same way as our previous approach right?

Is it necessary to have such a test? 🤔 getCommand() just calls the nextLine() function of scanner class. So by writing this test, we are essentially testing the nextLine() function which has not been written by us.

Is this package being imported because of the BusStops being set in the array?

Can this be removed?

Nice to have this getter here! Ensures that only minor changes will be needed for other features that made use of the route arrays. (hopefully 🤞 )

Maybe you can print out only the 1st and the last stop of the buses here with a "..." in the middle.

Oh yah! Okie!

Is this for future changes? Will the current program work without this?

I was wondering if the getCommand() of Ui can be used here. Just to avoid declaring a separate Scanner.


            String userBusRouteSelection = Ui.getCommand().trim().toLowerCase();


public class RouteMapCommand extends Command{

This class was removed in a previous commit. You might have to update your PR to reflect the new code and make changes accordingly.

PS: There will be more changes required once #28 is merged but those should be minor

Once you change the code according to the new classes handling bus data, you would still have a similar function that returns the bus route. I suggest you shift the printing to Ui class. See #26 for an example.

You may want to remove this class since it is not being used.

Since you have just one test, using createBusList() here is ok. If you have more than one tests for BusData, remember that calling it again might cause duplicate entries in the list.

A possible way to do such initialization is using a @BeforeAll method.

This is something I found from my research. Do let me know if there is a better alternative!

Nice! Using for loop makes the code look neater.

Another way to do this would be to store lower case names in stopNames and then use contains() and getIndexOf() functions.

Nice function you have there! Can be implemented in the code for other features as well.

can this import be removed?

Okie! Yah, I realized it wasn't running!


import seedu.duke.logic.commands.BusCommand;

import seedu.duke.logic.commands.Command;

import seedu.duke.logic.commands.RouteCommand;

import seedu.duke.logic.commands.RouteMapCommand;

import seedu.duke.logic.commands.ExitCommand;

import seedu.duke.logic.commands.HelpCommand;

import seedu.duke.logic.commands.AllBusCommand;

import seedu.duke.logic.commands.ListStopsCommand;


public class ListStopsCommand extends Command {


import static org.junit.jupiter.api.Assertions.assertThrows;

import static org.junit.jupiter.api.Assertions.assertTrue;

import static org.junit.jupiter.api.Assertions.assertFalse;

no tab here


Kent Ridge MRT Station-> Opp University Health Centre-> University Town


PGP-> Kent Ridge MRT Station-> Opp University Health Centre-> Yusof Ishak House-> Central Library-> Kent Ridge-> Museum-> University Town-> University Health Centre-> Opp Kent Ridge MRT station-> PGPR


PGP-> Kent Ridge MRT Station-> Opp University Health Centre-> Yusof Ishak House-> Central Library-> Kent Ridge-> Museum-> University Town-> University Health Centre-> Opp Kent Ridge MRT station-> PGPR


PGP-> Kent Ridge MRT Station-> Opp University Health Centre-> University Town-> Raffles Hall-> Kent Vale-> EA-> NUS IT-> University Health Centre-> Opp Kent Ridge MRT station-> PGPR

Might not want to add assertions to a public method

Wondering if we want to move it to a different class like #57

Can this be refactored into another function or maybe a function in Ui?

Okie!


logger.log.2

logger.log.3

logger.log.4


  * [3.3.Check for buses at a bus stop: ```/bus```](#33-check-for-buses-at-a-bus-stop)

Would it be better to call the Ui method to print the message here instead?

I see, okie!

Can you push this to above /descfav?


Introducing **Nav@NUS**, your new navigation assistant!

I think the bus routes have BTC bus stops oso?

Yah


where you have to type in commands.<br>

Maybe remove the figure numbers for now, since we removed it from other places also, if the figure is right below the content


shown in the figure, the home folder is found in the address path of "C:\Users...\CS2113T Empty folder".<br>


6. Your screen should show the start screen of Nav@NUS as seen in the following figure.<br>


        System.out.println("Oh no! Some of the data in FavList.txt seems to have been corrupted.\n"

                + "Most corrupted data has been removed :)");

Great catch!

Oops!

Oh no! We missed that

Comment not required

Hahah, okie! I will push these changes in a new commit!

Okie! I will follow that approach.

Thank you for the feedback!

Oh yes! That's a better way to do it.

Sure! I was wondering if those should be added if and when they are being used...


        BusStops.PGP,

        BusStops.KENTRIDGEMRTSTATION,

        BusStops.OPPUNIVERSITYHEALTHCENTRE,

        BusStops.YUSOFISHAKHOUSE,

        BusStops.CENTRALLIBRARY,

        BusStops.KENTRIDGE,

        BusStops.MUSEUM,

        BusStops.UNIVERSITYTOWN,

        BusStops.UNIVERSITYHEALTHCENTRE,

        BusStops.OPPKENTRIDGEMRTSTATION,

        BusStops.PGPR

isValidBusStop accounts for it. So this is redundant

Minor change needed for parser class so that the child classes can make use of splitCommand() method

I realized that this will effect other features involving the use of the bus route array in main. We should remember to make appropriate changes in other pull requests that are open

Since #13 is linked to a different feature, you might want to create a new issue solving a different user story. For example, "As a forgetful person, I would like to know which buses are available in my zone, so that I can avoid entering the wrong bus."

Need to add more tests to account for the new command classes added.

Is this supposed to fix the issue #18 ?

Seems that this issue should have been closed already!

This issue has be resolved.

Issue can be closed now!

Also contributes to #54

Fixed by #59

Fixed by #52

Fixed by #69

working towards issue #55

fixed by #122

Can we close this issue now?

Done!

fixed by #136 #137

Might have to pull from master 1st. I have updated /descfav. Not reflected here

Thank you for the feedback! Will work on these soon!

All diagrams have been drawn using plantUML. The .puml files are all stored in docs/DG_Figures/

Your most frequently typed bus stop name is displayed when you restart the application

But it is a good point, we must make a clearer mention of it in the user guide. @Johnson-Yee

I think we have this exception handling issue open for freqList.txt.

Need to add this for favList.txt as well! @EthanWong2212

This command is meant to be like a search filter (the way you would search for documents in your folder), so that users do not have to look around for correct faculty names. But this may get a bit confusing for a few users. @mrwsy1 what is your take on this?

Will consider adding that in! Requires a bit of thought as some stops have a few common names which would have to be accounted for.

parameters "only" was not mentioned. This was a statement catering to notes about parameters. But we should consider including the same for commands as well since some users may find it confusing.

Same as #175

Good point!

To be changed!

Same as #175 and #169

work in progress

Good point!

I think we can leave such prompts out of the output of commands?

ahaha sure! the easier way out 😉

same as #173

we might have to implement our previous idea (isValid) @Lezn0 your thoughts?

Same as #162

Was changed a few hours before the PED. refer to this link

Great job!

Oh this is a nice addition!

Good way to avoid magic literals!

Thanks for this! I think this implementation makes more sense!

Oh right! My mistake on the primitive versus object type for the boolean variable! Thanks for helping me edit that!

I think the new naming you have here is much clearer too!

Great work! I think the logic is fine for the quiz implementation!

I think we can implement the parseAnswer that we currently have in the Quiz class into here in future! But this will work for v1.0.

Quiz object should be constructed in future but this will work for v1.0!

I think it is better if the parser class and its interface could be packaged into their own package, which can be called parser

Great implementation for v1.0! Do take note that we have to handle the case where the input provided by the user is unfavorable though, such as if the input length is not 3.

Wow! This looks great!

Do you think making this into an abstract class would be better than an interface because other command classes will eventually inherit from it instead of just specifying the behaviour?

Nice! I think this implementation is great!

Thanks for this! It enhances cohesion between hint and question!

Thanks for this too! The flag will now correspond to whether the question was shown to the user or not.

Understand the reason for this! For future versions we will be able to handle multiple-topic quiz.

I think this should be add instead too!

Since optionList is unused here, do you think the execute method's parameters in the Command abstract class can be changed?

I understand that you have incorporated question.markAsShown() function into the getDescription method of question now, however I feel that we should separate them out to enhance SoC and cohesiveness for the method. Will be doing that and hence for this part over here, the questions will have to be marked as shown separately.

Just a suggestion, but I think what you have now works too! The suggestion is just something to think about for future implementations!

Remember to remind @anqi20 about this again @josephhhhhhhhh !

Great! Looks like a working product is right on the way!

Logic seems great for this class!

Naming of tests are great!

Thanks for helping the team with this change!

Thanks for this! Careless mistake on my part haha.

Do you think this might be better if phrased as "Please enter in 'help' to get the list of available commands." ?

I think for all new lines we should use System.lineSeparator() instead as it will ensure that it works for any OS.

Great job!

Can add a new line between the two parts but great job nonetheless!

I think these should not be returning new IncorrectCommands but should be using the ui to print the error instead. the IncorrectCommand will not be executed too.

Great job! Thanks for changing this!

Thanks for changing this too!

Great work! Logic seems fine to me!

Yup, I agree! I think most of us will have to do that too and that could fall under an issue for v2.0.

Great job chaining the exceptions!

Looks good with the naming!

Thanks for adding this in! Looks good!

Not very sure if you need the arrows that do not correspond to a method call or not. Also, the return values should be dashed lines.

Looks good! However, should the multiplicity for the association between OptionList and Displayable be 4 instead of *?

This is a great idea! Thanks! Now our code looks way cleaner

Great use of constants to avoid magic literals!

Looks like you have tried to cover as many possible scenarios as you can in these tests! Nice!

This is much clearer now! Great!

Great use of System.lineSeparator() !

Great change! We should have just use the method to print out messages as it already exists.

Hey! Thanks for the quick response! Able to make timely adjustments because of that. Have made a new commit with the necessary changes! Do check it out if you are free!

Sure! Working on it!

Alright noted, happen to have it here because it is linked to the QuizQuestion class.

Alright I think a better fitting name would be QuizQuestionsManager as it manages the quiz questions instead of simply being a list.

Initially I wanted the getDescription method to only be getting the description of the question, but it is also indeed true that once getDescription method is called it is clear that the question is shown too. Will be changing this! Thanks!

Working on it!

Oh! Sorry! This comment is supposed to be deleted from the commit as it was from a previous version! I will change it now.

@kstonekuan has pointed out the mistakes with these lines of code. Thanks!

Alright! Thanks! I will change accordingly!

Yep, thanks! It is now updated as such in the new pull request.

LGTM! Good job overall!

Working on it!

Yup! @anqi20 I think you will have to take a look at how hint is implemented and related to the storage class and copy that logic into the explanations class.

Not necessary as it's not used.

Good idea to save all white-spaced separated characters into inputs array.

Should throw an error.

@samuelchristopher consider what happens to projectIndex pointer if project gets deleted.

Not sure whether should throw an error for this.

Should be printing error messages.

Do we need getter when we can access class property directly?

Will this error ever be triggered?

Can add some further testing to check task list after adding and deleting of tasks. Also check that switching to another project changes the list of tasks.

Should use the inputs array that Riaz implemented where he split the inputCommand into multiple words already.

How do we handle deletion of members with duplicate names? One way could be not to allow duplicate members with same name to be created.

Consider making message type more descriptive perhaps by naming the error(See below). Error messages are clear.

It is hard to tell from the messageType the error. Example Naming: AddTaskWrongView

I feel like we shouldn't use variable "description" for both project name and description as it will be confusing to use one variable for two different purposes.

Same here. I feel like we shouldn't use variable "description" for both name and description as it will be confusing to use one variable for two different purposes.

Should not be "Task View"?

I feel to make it more convenient you can return name, description and deadline here.

Then, here just call toString to return name, description and deadline here.

Ya Ive left similar comments in your previous pull request, do check it out.

Same point as Shreyas mentioned...members should not be access directly.

isNamePresent should be renamed. It's to serve as an output.

Project.members

Good renaming of variable

I don't think this should be private too

Can be simplified to project.getTask(taskIndex)

No need for this function after above comment.

I feel that selectTask is just retrieving task using project.getTask() followed by an output message. There's no need for this function as output is handled in Command class

But parse() only throws DukeExceptions and not NumberFormatException?

How does it catch Integer.parseInt?

iirc prof mentioned we are not supposed to catch NullPointerException. Checked online and seem to be the case as well.

Not urgent but for consistency are we supposed to use the Ui class you implemented?

Hmm what is switchViewModes for? There's already home command to switch to home.

Consider assigning tasks to TeamMember here

It becomes important for members to keep track of tasks so deleted members will no longer be attached to tasks.

Are we printing lists here or in Ui? I actually don't mind either but should be standardised. Not a high priority as of now though.

Just to clarify, this command activates the Project View both Samuels are doing right?

Why is the try catch shifted from parse to executeCommand?

Shreyas says in another pull req "as riaz mentioned I think we don't need to put a try-catch block over here since we catching it in the main class, we just throw the exception in that method"

But parse() only throws DukeExceptions and not NumberFormatException?

Agree with this

I feel this is confusing. What is the difference between isDone and getStatus?

Agree

Yup thanks Sam

The user guide has no deadline for projects. But if simple, can just do

Yup done!

My thinking is it would be clear where this method is called ie. project.getTask().

What do the rest think?

No it can accept more than one parameter. The split here is to separate commandType from the parameters. Inputs[1] contains the whole string beside the first word.

As of now, the function already loops through and extracts all params.

Hi Sean @thatseant , let me answer the points:

There's no way to display project view other than selecting project.

Is there a need to display project view other than selecting the project?

Multiple members are not shown.

Currently, each task can be assigned on member.

Tasks are not numbered so it is hard for user to see index of tasks.

Oh okay! I have built this from the mock-up that we approved and I did not include the index at that point. Maybe for the next iteration we can add an index column? For now, users can use the list command?

Hmm Sam regarding point 1 and 3 yes effectively the project view replaces the list command which doesn't show anything useful.

If not lets say in my program i made a few changes such as adding deadlines to my tasks. In order to view the updated changes:

I have to return to home via home command
then select the project again via select just to view the changes

I propose just showing the project view when user types list command

Maybe in the while loop you can switch it to a variable called: isEmpty or something instead, then you update that variable through a method which returns true when noteTitle.trim().length() == 0

These 2 lines can be abstracted out into a single method for better SLAP

Can extract into a method named "isValidTitle" or something

This can be extracted into a method as well, the contents in the brackets of the "if condition"

Can be extracted into a single method to improve SLAP

Same thing, can be extracted into a single method

Extract into single method called "checkForOutOfBounds" or something

Your test names I think you can look at the naming scheme on the module website - SUT name_Inputs_Expected outcome - style

Maybe you can try breaking this part up so that its abit easier to read

Maybe each if statement can be put into its own method

The contents in the if statement can be simplified into a nicer name I think, like "isEqualToDate"

This command can be put into a separate class for reuse by the delete class

Same as above, can be put into another class to be shared

This as well

Can split into a few smaller methods

Repeated in 2 classes, can be shared also

I think you missing a word here

Maybe can simplify the condition into another method that returns true false to make it a bit more readable.

Same here for the JavaDoc comment

Should the logic for like determining which messages to print be done not in the Ui class? I'm not very sure also, want to know your opinion

This line can be sent to Ui class - Pass in the "i" and the "note title" into a Ui method to print for you

Same as above

Can simplify the while loop conditionals


        while (!isEqualY && !isEqualN)

Same as the above

Simplify this also

Simplify this conditional

Check for the extra white space

Maybe can just add some white spaces to separate the different logic to improve the code readability/aesthetics abit! Same for the other edit commands below.

Contents of if conditional may be extracted into another method to return true or false

Same for this if conditional statement. Can be extracted into a method if you can find a name for it

Same here, for the if conditional

Maybe this method can go into Ui, will need to check with the rest

Actually, maybe we could have a class accessible variable for NoteHistory so that any method can access it at any point. When the execute is run, we set the class variable as the passed in noteHistory, this way we do not need to pass the noteHistory object down through multiple methods.

Help move the storage.saveTasks() from after all the catch blocks to after when the taskHistory.pushOldCopy() method is called - (Line 74 in this case) for the event and lesson edit classes also.

Move the storage.saveTask() method to the line below this.

Can we make this a Ui method

Maybe instead of setting the editNumber through the constructor, set a method called setEditNumber() in the Task class itself instead.

Hmm yea good point. But I think what Johannine is trying to get here is separation of concerns, where we have a class that just deals with the tracking and updating of the history of tasks. I guess this is up is really up to choice, which way do we want to push towards more.

Actually is there any case where we will not decrement the recurrence by 1

Thanks for the catch, I am going to change this issue.

Possible, I realised the name for this method may not have been the best, the idea was that I have a method that checks if both files exist - If they do, we will skip straight to file reading. If any of the files do not exist, we will proceed with save creation.

I have updated the method name to "checkIfBothFilesExists" to better reflect this.

Alright, I have refactored the entire method into individual methods to improve the SLAP

Alright, ive tried my best to refactor the function into a single method

Alright, I've changed this, thanks for the suggestion

Yep, definitely, but idk where to put it for now, so I wont move it yet

Alright, changed it, thanks

Alright, updated the code accordingly

Closed by #75

Point 5.0 Product Scope in Developer Guide missing preface
Add new Undo function into Guidelines for Manual Testing
Fix typos in section 3.2.2 UI Component

Is this supposed to be inputChecker(userInput) and not !inputChecker(userInput)? If the user command is valid then it will continue asking for user input?

Is HelpCommand supposed to be the only one in Pascal case?

If removing getter function, then the feedbackToUser attribute should not be private.

Should be output == null because TaskManager/ModuleManager returns null if the respective lists are empty.

TaskManager's edit function takes a new Task object to get around needing an edit function for each property of task, especially when we will be adding new task properties in the future. Is it possible to reverse this change here?

What are these edits for?

Command classes should only call methods or store and pass information, and should not have standalone methods. Ideally, your timetable command would call 'TimeTable.generateWeekTable()' (and others) and not have its own method for doing so.

Will roll these methods into the new Lesson class

The logic in this method (and the next method below) will be moved to the new TimeTable class

Do we actually need quotes around the old and new module codes?

Can we actually use this format for date?

Missing a space between opt and args

Download instead of down?

Space after colon. Also, the command is bye instead of exit no?

Should add an explanation here that the task_index can be found from list -t

Should be bye?

The "Add a module to the timetable" line should be removed since this command is continued on the next line.

Just checking - I thought we implemented timetable commands already?

An example like 30-12-2020 will probably be better since 2-2 is a bit ambiguous about month and day.

Inaccurate deadline format here

Should give a short explanation of task_index and how to find it

Space between >opt> and >args>

Should be delete -t 1

Should be delete -m CS2113T

edit -m CS2113 CS2113T

Should also add context that only mods that can be found on NUSMods can be added here

Should be >task_index>

Should be edit -m >module_code> >new_module_code> to stay consistent with UG

Space between >opt> and >args>

>task_name> instead of >args>?

>module_code>?

Space between >opt> and >args>

>task_index> >task_name>

Why is "Undo" help inside TimeTableCommand?

All the >...> fields should be named the same way as in the UG. Refer to the above comments to see examples.

Okay, I will add that in a later PR

There will be a total of 27 enums which will have names like MOD_SUCCESS_TASK_FAILURE_TIMETABLE_SKIP which I think is less readable than this. I think this is an acceptable loss in readability since it's only being passed between 2 functions.

Also I'll need 27 conditional branches in order to return the right enum 😨

Seems like there's a lot of changes made... Can we get a more detailed description?

Checks seem to be failing due to missing portions from other parts of the code base. Parser code seems good to me.

Please update branch

Update branch please

Please update branch

#70 WIP

Seems like UG updates are included in the other PR? Closing this one.

What's the bug?

Try using this in Message.java instead


public static final String ICON_DONE = "\u221A";

public static final String ICON_NOT_DONE = "\u0078";

What's with the build.gradle update?

Fixed by #150

Nice catch, fixed by #152

Can't seem to replicate this one. How about you guys? @Aseanseen @amalinasani @tobiasceg @wangqinNick

Fixed by #153, thank you

Fixed by #153

Perhaps you can explicitly list out all the imports instead of using * to pass checkstyle

maybe remove unused code?

whitespace after while

whitespace before { ?

Maybe this method should be Storage class since the Create Command should be focusing on dealing with the creation diet session

perhaps this can be extracted out to the printResponse() method

Printing help in workout session search does not seem so right 😅

Perhaps can implement the UI printing here also

Should the name of array type variable in plural form (i.e. inputs, exercises)?

Should the boolean array name sounds more boolean (i.e. hasEndedWorkoutSessions)?

Workout Session is under Workout Menu, so is it clearer if we show "Workout Menu > Workout Session >>>>>" ?

This Confirmation method is duplicated in multiple places, so maybe we can extract them out into utils?

Should we delete those commented out lines?

For consistency in printing, can change here to "delete [MOVE_ID]"

And also here to "search [NAME_OF_MOVE]"

Actually, it's used in workout manager and workout session as well. So can merge this PR first then I'll create a util method for that and you guys refactor your code accordingly

Since they are final variable, should they be in all uppercase form? Or just remove the word "final"?

The command words can change to the constants created

For consistency, should it be "Workout Menu"?

And "Main Menu" here?

Parses?

an integer index?

can consider adding a blank line between description and @param to follow coding standards? And add punctuation behind each parameter description also. It's in the "note in particular" session.

This format is also repeated in WorkoutSessionList, so you can consider refactoring it to a constant.

Should this be "Please make sure ... are non-negative numbers" since move like push-up can have weight of 0?

The FILEPATH can consider using the path constants created in seedu.duke.Constant

Should this comment be updated also?

And also is the "wrong format, please enter ... [command format]" part a bit redundant in the message?

Since WorkoutSessionUi extends CommonUi already, should the line "private static CommonUi printer = new CommonUi();" be removed and use the "showToUser" method of CommonUi?

You can make use of "showUser" method in CommonUi. It's also static.

Can consider shortening this part to this.calories = Math.min(calories, 200000);

Should the title be "Searching for a Move from the Current Workout Session"?

Change this title to sync with the title in table of contents

and change the anchor to >a id="ws-search">

The feature description does not match

Fixes #62

Great work in refactoring! 👍

Great enhancement!

It would be better if "commandArgs" is used instead of "arg" to standardize with the rest

It would be better if you initialise the Chapter object in the ReviseCommand

follow the format of methodUnderTest_inputGiven_expectedOutput for test methods

out.println will do for this line

It would be better to use .equals instead of == and != for comparing the string

instead of checking if (access.getModuleLevel() != "") first, you can do something like this:

if (access.getModuleLevel().equals("")) {

// print error message

return;

}

// rest of the code that should be executed

instead of checking if (access.getChapterLevel() != "") first, you can do something like this:

if (access.getChapterLevel().equals("")) {

// print error message

return;

}

// rest of the code that should be executed

instead of checking if (access.getModuleLevel() == "") first, you can do something like this:

if (!(access.getModuleLevel().equals(""))) {

// print error message

return;

}

// rest of the code that should be executed

will getting the chapter in ReviseCommand be better? means you pass the chapterIndex in the ReviseCommand()

This method is not needed, it has been changed to loadCard

Would the original way be better?

Admin admin = new Admin(storage.loadModule());

Then don't need to import the ModuleList class

Should all these be under Ui class instead?

Should return ListDueCommand()?

This method not needed as well

Do not need to pass in Storage object

Yes

I think for this, you can have a return after line 41 so that you would not need the else part. Then the rest of the code will follow. Something like this:

if (dueChapters.size() == 0) {

ui.showToUser("You have no tasks due today! Please check back again tomorrow!");

return;

}

// rest of the code

you can use ui.showToUser

maybe you should check if the user entered a valid argument? for example you can check if the user entered a valid date, and if the user user entered 'history', you can pass today's date as the argument

Maybe you call createHistoryDir in the method createAdmin in storage.

And for createHistory, you can call it in the method saveHistory

instead of checking for history here, you can check in loadModule in storage instead. so that it wont add the history folder as a module object

instead of checking for dues here, you can check in loadChapter in storage instead. so that it wont add the due folder as a chapter object

if the due date is null, you can show something like "No due date" instead of "due by null"

you can use setIsModuleLevel() to do this

you can use setIsAdminLevel() to do this

you can use setIsModuleLevel() to do this

you can use setIsChaptereLevel() to do this

these 2 methods not needed anymore

maybe you can do throw new FileNotFoundException(message) then it will show the error at Kaji.java

Perhaps you can consider changing the method name to getPortionSize.

Perhaps you might want to leave a blank line at the end of the file.

Maybe you can consider adding JavaDoc comments to some of the methods in this class.

Perhaps you would like to split them into different tests using assertEquals, assertTrue, etc for testing and using a three-part naming style for each method e.g. methodUnderTest_inputGiven_expectedOutput

Perhaps you would like to leave a blank line at the end.

Maybe you can consider using a specific import rather than a wildcard import.

Also perhaps you can consider using the three-part name format for the naming the testing methods: methodUnderTest_inputGiven_expectedOutput

Perhaps you would like to consider this for the other files as well.

Perhaps you can consider splitting them into individual tests and the consider naming each test in the following three-part format: methodUnderTest_inputGiven_expectedOutput

E.g.


@Test

public void getCalorie_food_returnsCalorie() {

    assertEquals(480, testFood.getCalorie);

}

Also, for the assertEquals method, I think the parameters are (expected, actual) so you might also want to consider switching the variables around.

Perhaps you would like to import specific methods rather than using wildcard import.

Perhaps you can consider splitting them up into different tests using assertEquals and assertThrows for tests that throw exceptions.

Perhaps you can consider leaving a new line at the end.

Consider breaking this up into different methods.

Maybe consider adding a semicolon at the end of this line.

Perhaps consider adding () after clear so .clear()

Maybe you can consider using ui.printDatabase(manager.getDatabase.getFoodList) because I think printAllData method is used mainly for debugging purposes.

Hi! Perhaps you would like to add in the name parameter when constructing the person class as well as currently the first input parameter of a person is actually their name.

Perhaps consider using break instead of return after each case switch.

I am not sure if this will be an error, however, I have changed my printFoodList input parameter to a String instead of a FoodList, maybe you would like to add in .toString() behind the getFoodLis()

Sorry for informing you late, last night I decided to change the method name to ui.printPersonInfo(String personInformation) to keep things consistent and shorten the method name, so perhaps you would like to make an edit as well. Also, I can't seem to find the variable personInformation that you passed in as a parameter maybe you would like to input getPerson.toString() instead as the method takes in a String.

I think foodList might not have a get method so maybe you can consider shifting line 114 up as line 114 returns a String representation of the deleted item and you can just pass this String into the ui's printDeletedFood method.

Perhaps you may want to change this as Ui does not have a divider method.

Perhaps consider changing female to an if-else case and the else case can throw an exception. Maybe you would like to consider this for activity level as well.

I am not very sure if an update would be good/necessary for this file but perhaps you can consider.

Perhaps you might want to make a specific imports rather than wildcard imports.

Perhaps you would like to consider using the three part naming style.

Perhaps you would like to handle cases where the activity level is high or extreme and code an assertion for the default case.

Perhaps you would like to use an assertion for the default case.

Maybe you can consider using current weight instead of original weight for the calculations.

Perhaps you can consider adding a blank line at the end. Same for the other files.

Perhaps consider leaving a blank line between headers and content.

Perhaps consider using the three-part naming style for the tests.

Perhaps consider adding a case for male gender.

Thank you for reviewing!

Reasons behind the implementation of setter methods: Possible future extensions that enable users to make edits to their information in case they have inputted it wrongly or if the information has changed over time. (Editing function is not currently available in this iteration as the user can restart the program to reinput the information but this function could be considered in the next when user data is saved and cannot be edited otherwise). However, if this is not implemented in the next iteration and is unnecessary, it will be removed.
Will add current weight field next iteration

As the Food class (functional but without JavaDoc comments) are already merged into the master branch of the team repo, maybe you can consider pulling the team repo into you local repo and merging the master branch to your branch (take note not to do it the other way around) so that your branch is up to date with the team repo and this would also likely minimise any possible merge conflicts.

Merging of Calculator was done in another PR #19.

Satisfied by #42

Merging of Main. Parser and Manager done in #44

Satisfied by #104

I/O testing will not be used.

Once we have Ui class we can consider moving these lines of code there instead, and call the required methods. Same for viewProfile().

Try to be consistent with the spacing between methods? And add the whitespaces in the method names, eg

public Exercise(String name, Calorie caloriesBurnt) {

For this class and the other classes as well.

I think you haven't deal with the case when the user enters bye. Maybe use a boolean isExit, equal to c.isExit() and use that as the condition for the while loop so that the loop can be terminated?

Remove the whitespace between Fitr and the open bracket, i.e. public Fitr(...) {

I believe the exception class is not yet created, when it's created it should be FitrException (follow camelcase)

Need to override isExit() method

Run the checkstyleMain and fix the coding standard violations

Use getBmi instead of getBMI

Maybe consider using primitive types instead.

Would it be possible to put this in a new class? Something like a DateTimeManager class?

Need a fullstop at the end of the sentence, otherwise the checkstyle will complain.

Need to change the text - you're reading the tips file.

If we move the tips.txt file into a resources folder, then I think there isn't a need to pass the path of the file to the constructor since the location of the text file would be fixed. Can consider removing it from constructor and just define src/main/resources/tips.txt above.

Wouldn't be necessary - the file is not user generated, and it would have been packaged with the JAR file already.

Check indentation

Will the user be entering a command to get tips? If not I don't think it should be a Command class... looking at the main class it seems like it's not, it shows up whenever the program first runs. I think can consider not extending Command and just use a standalone class, maybe something like TipManager?

Feels like this can be simplified, since the tips will be shown when the program first runs, and the user isn't going to be typing a command for tips. See the comment in the TipCommand class..

See the comments in the Storage class - I don't think its necessary to include a file path of the tips file since it's going to be included inside the JAR file, so can consider removing it from the constructor here..

I think should put in a single line, not separate

Since the class is not a command anymore, maybe can consider putting it in another package? Maybe a tip package?

Got two different caloriesInFood here...

Isn't the return type of calories an int?

Are these commented lines still needed? If not needed then maybe can consider removing them?

Same here - maybe consider removing them if not needed?

Need to add public in front

Need add public in front also

I think using assert like this is abit dangerous?

I think its printCustomMessage right? Not error

Can consider extracting this out as a method?

Fixed in #24

Finished implementing the loading and saving of files for user's profile, food and exercise lists.

Checks failing because of the AddCommand class.

Already implemented, thanks

See #167

@gohsonghan98 can consider one-shot commands, eg edit name >name>

Would be good if an extra line is added after EOF.

All good now!

Perhaps we could standardize our logger to be something along the lines of private static Logger LOGGER = Logger.getLogger(">CLASS>.class.getName()");

Remember to add a new line for EOF, otherwise, LGTM!

Perhaps use of a constant would provide more clarity.

Good use of logger!

Perhaps a constant would provide better clarity?

Introducing a constant here would be good?

Might want to add a new line to EOF here.

How about a new line here?

Would it be better to just gitignore the entire folder rather than individual sub-folders?

Perhaps some meaning could be added to those numbers.

Perhaps the phrasing for this message could be simpler

Perhaps add a new line before EOF.

Great work extracting datetime class!

Perhaps we could standardize the amount of periods we use for logs, and when to use them?

Great work on updating the runtest file!

Alright sounds good!

Perhaps you could standardize params naming to be PARAMETER_NAME?

Great work in writing tests!

Great work separating long strings into multi lines.

Is it good practice to print test results?

Great work refactoring them1

Good tests!

Great addition!

Perhaps add a new line here

Would a new line here be good?

Good work with refactoring!

Great work explaining!

I like your example

Updated, thanks!

Fixed that, good catch.

Thanks for the reminder!

You're right, shall leave the fix for the next update!

Good catch, removed them!

I apologise for the delay in the review. Thanks for working on the UG!

No worries, have a good weekend!

Hello,

This was actually intended as workspace is meant to only include alphanumeric characters and or spaces.

I will take note to update the guide instead.

Thank you.

Indeed, this might be something we can consider adding in future versions.

@OngDeZhi what do you think?

How should we fix this, team? In my opinion this is a font issue but perhaps we could provide more clarity on our end.

@AY2021S1-CS2113T-F12-2/developers

I think this can be removed?

InvalidInputException?

I could do that next time if you want? xD

Okay, sure!

I think imports should be listed explicitly, as stated in https://se-education.org/guides/conventions/java/basic.html

Sooooooo much neater omg, thank you.

Good effort in making this section neater xD

Probably should not have wildcard imports...

argument* , same for the others

Trivial, but you may want to use NOTEBOOK_DELIMITER, SECTION_DELIMITER and PAGE_DELIMITER respectively for this.

oh yes hehehe that's a better description than mine xD

missing the link to the delete function!

oh yes separating the commands for the different modes seems like a good idea! 😀

I'm not sure if we should leave this as planner mode.. I think we should standardise what we call it since it's called timetable mode in our code.

hehehe thank you for helping me fix this

btw, I think you need a gap between the # and the words, i.e. "### Listing tasks: list" instead of "###Listing tasks: list", in order to make that a header.

oh yay

I think you should remove this line...

I think this should be removed ?

Good job!

it's intended. xD

nope, you can select a notebook anywhere.

not sure if you wanna add select /nCS2113T /sChapter1 /p1 into this section because you can actually do that in our app...

also not sure if you noticed but it's prerequisites... HAHAHAH I think you just copied and pasted for all the prerequisites part so it's just prerequistes..hehe

oh wait as in the date format must be dd-MM-yyyy. It's just that both 1) add /t test task /by 2020-10-10 2000 and 2) add /ttest task /by2020-10-10 2000 will work! notice how there's a space between the delimiter and user input in 1).

OHOH I SEE WHAT YOU MEAN NOW YOU'RE CORRECT. sorry for the confusion xD

HAHAHA NICE.

It still works actually xD

some are non-academic related * ?

Hmm, I initially didn't have that there but IntelliJ threw me an error. xD I clicked on whatever they suggested (which is return null;). No worries, I'll go have a look at it again 😄

Yea, sure. That can be done. 👍🏼

... Omg that is true HAHAHA okay, I'll change that.

Yea sure, that would be nice.

...Right. I'll delete that line.

ah, definitely not. I'll go change that.

I did something similar in my own ip as well. xD See if the others have anything to say about it then?

I'll add this as a //TODO for now

I'll just add this as a //TODO for now

added as a //TODO

What do you suggest changing it to? xD Or you mean I should just replace that line with sth like if (notebookIndex > 0) ?

ooooooh the -1 is thrown when the notebook/section/page cannot be found (when the user selects a notebook/section/page that doesn't exist)... so you don't like that the -1 is being thrown is it ?

Sure, I'll edit that before merging.

Do you think it sounds better if i add "Having all your notebooks in one place can help organise your notes and thoughts better! " right after that?

honestly, not sure if i should put it there when we haven't used the mascot anywhere else... and it's not like the mascot is even necessary

honestly yea, the >br> is doing the magic xD

Yayyyyyy thank you.

should fix #62

Fixed by #91

if you think it's good enough then merge it please 😛

Should fix #119, #120, #121.

The IO test will fail now since there's a welcome message right? Or is there something wrong with my code HMM

I don't think it has anything to do with the IO part?

I updated the IO part after adding the welcome bit... haha maybe you're missing an import statement somewhere?

Ready for review pt2 xD

Fixes #112, #116

newer functions not in CliCommands diagram

utterly confused and unable to reproduce the same error.. o.o

The expected format is actually delete /nCS2113T /sTesting /p3 . . . I guess we can change the format then. xD

The expected format is select /pnumber e.g. select /p2. It is also stated in the error message. but it's okay xD

@yAOwzers in the group, they mentioned that maybe we should just change the whole format for calling the command.. like just type in the page title instead of the page number ...

should close #153 , #155 , #158 , #163 , #164 , #165 , #166 , #167 , #169 , #171 , #172 and #178

According to the UG, the file should be data.csv. This was my mistake that I did not fix because it was temporary test code.

the correct name here according to the user guide is load.csv. This was my mistake, eventually we should allow the user to change what file they are loading

There is a possibility of an empty save file, should initialise maxNumber to the default of -1?

Much cleaner code than the original

found it a bit weird that the argument order is not the same as add task, I know that the number is a required argument so its in the front, but it might look a bit messy to others.

I would have done the same, but am also not too sure about the correctness

Done

Sounds good!

The TaskList initialises the maxNumber as -1. We can either change it here or there as long as its standardised. I suggest changing it here because I used this in some of the test code and allows us to check fo rpotentially unused TaskLists

This if else block can be replaced with a switch block to improve readability

This is a good idea

small nitpick, i can start from 0 then go to i>6, functionality wise is the same but might be more readable for others

not sure if we should support having tasks with the same task number, as long as the cloned tasks are not saved should be ok.

Nice cleanup here, eventually should be moved to Time class

need more parser code to change this later

maybe can use the string "+-------"?

same thing as line 104, maybe can use only 1 string

not sure if this is going to be flagged in the checkstyle

Something similar is in 115, could consider making it an attribute or a method call from the Task/Time class

I think that keeping these this here is better, it keeps calling bits and pieces all the time. If we do eventually do refactor it, the class needs to put the entire timetable in one shot

we can just throw an error that says that the tasknumber is conflict with another, I'll handle this in the allocator PR

awesome

Add a check if the taskRecurrence is Today or a particular date, probably don't need "every today" to be printed out

Basic functions are covered that's good. Cleaned up the diagram a little bit, check it out and tell me what you think

title List

actor ATHENA

ATHENA->ListCommand: execute()

activate ListCommand

ListCommand->*ImportanceFilter:

activate ImportanceFilter

ImportanceFilter-->ListCommand:

destroy ImportanceFilter

ListCommand->*ForecastFilter:

activate ForecastFilter

ForecastFilter-->ListCommand:

deactivate ForecastFilter

ListCommand->*Timetable:

activate Timetable

Timetable->+TaskList: getFilteredList()

TaskList->+TaskFilter: isTaskIncluded()

TaskFilter-->-TaskList:

TaskList->+ForecastFilter: removeExcludedDates()

ForecastFilter-->-TaskList:

destroy ForecastFilter

destroy TaskFilter

TaskList-->-Timetable:

Timetable->Timetable: populateTimetable()

activate Timetable

deactivate Timetable

Timetable-->-ListCommand:

ListCommand-->-ATHENA:

Rest of this looks good, one small suggestion to change this to TaskList->*Task: , it makes the arrow point directly to the block at the top.

Might want to include the actor here

need an activation bar here

activate AddCommand

Cleaned up the diagram a bit


title Edit







EditCommand->+TaskList: editTask()

activate EditCommand



TaskList->TaskList: getTaskFromNumber()

activate TaskList

deactivate TaskList



TaskList->-TaskList: createTask()

activate TaskList

TaskList->*Task:

activate Task

deactivate Task





TaskList->TaskList: checkClash(possibleEditedTask)

activate TaskList

deactivate TaskList





TaskList->+Task: edit()

Task-->-TaskList:

TaskList-->-EditCommand:

deactivate TaskList

Nicely done. one small edit will be making the activation bar for the ListCommand

avtivate ListCommand

I gladly accept this nitpick

I refactored this part and deleted the overloading because it was not being used

It deletes the correct thing, but I have to agree that the naming is a bit off

The overloaded method is to distinguish tasks that require a new index vs a preloaded task with its own assigned index

I'll just keep it in for now until Alston is done with the main Athena

I'll change it to isFlexible

I need the sorting according to time so ArrayList is actually a bit better for this scenario, I might need a bit of help with the time sorting, not very clear how to do it

Ok will change soon

Its the code coverage report files, they might appear again so I put it here so it won't be added accidentally

That works too

I changed it to DAY so we do not need to make a new Forecast value

Eventually we can assign it in the settings, its a magic number for now

It actually quits if it hits this part so the user would not notice

I was thinking the same thing actually, it is a bit difficult to store 2 separate sets of data in one csv

On Sun, 4 Oct 2020 at 10:05 PM, Daniel Lim Wee Soong > notifications@github.com> wrote:

@Kafcis >https://github.com/Kafcis> I think we don't need to store the timetable generated from the task list in a file, because the same timetable can always be generated from the same task list. Looking at the scale of this, there won't be much impact in performance if we generate the timetable when the program starts. But if you wanna do it, you can use getTimetable() to get the week's timetable.

@alstontham >https://github.com/alstontham> The timetable given to the Ui will be a list of TimetableDay objects, each containing a date and a list of Tasks. So you can work on printing that now.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub >https://github.com/AY2021S1-CS2113T-W12-2/tp/pull/8#issuecomment-703260677>, or unsubscribe >https://github.com/notifications/unsubscribe-auth/ALNIBIWF2UI3JON5UFBE72DSJB6J5ANCNFSM4SAG45PA> .

I wanted to avoid changing Amanda's code, but I can do it as a method

I think this is safe to merge, most of the checkstyle problems are addressed in the next PR

Duplicated

That sounds logical, I will do that as well for the subsequent PRs too

we did a quick fix before v1, I'll settle this in another PR

Yeah it's done

It actually goes to that part because I don't want to keep checking if the task exists before I delete it. I think its bad form but it prevents the code from having too many layers. At the moment it is already like 3 layers of nesting at some parts. The comment is just to make sure it doesn't trip the checkstyle

The format of the sequence diagram can be changed and I don't know which one we want to use. I can put them in quite fast

The time allocator test has to be completely redone, I'll pull from your branch and work it out

I can't replicate this either

Confirm, might need to update UG to clarify

Could this method be generalised? It doesn't necessarily have to be a command specifically used when saving and loading files.

I think some quotation marks could make this clearer, and in the end of the sentence, I think you might mean all other cases.


     * @return Boolean true if string is "true" (ignoring case), and false for all other cases.

Should this be split into two methods, one for each save file? Might make it clearer.

Same as above, this could be split further into 3 methods, if it makes it clearer.

I think the JavaDoc comment for this function, and the variable's descriptive names, make this comment a bit unnecessary.

Similar to a previous comment, the JavaDoc comment and good variable naming makes this comment seem unnecessary.

I think these functions only differ in the parameter you are passing in. You could consider making it one function?

Could the method be renamed to better reflect the variable you are creating?


                String noteToSave = convertNoteToString(note);

Same comment, you could consider changing the method name to better fit what it does.


                    dataToSave = convertDeadlineToString(task, taskType);

You could consider this for the methods below too.

Consider changing these to non-static variables. Such that we would be accessing the object for the getType() method instead of accessing the class.

Could consider using the DATE_TIME_FORMATTER from the FileCommand class.

There's no need for constants to be private, and if we ever decide to change the DATE_TIME_FORMATTER there would be less stuff to change.

Could be renamed to make it consistent with checkForNotesSaveFile and other variables


    private void checkForTasksSaveFile(File saveFileTasks) {

Since the LifEasier class already instantiates a TimetableUi, could consider just passing that object into the execute command instead of instantiating a new one.

Could consider putting this into the seedu.lifeasier.ui package with the rest of the ui classes.

You could also consider leaving it like this, and in LifEasier creating a new DisplayScheduleCommand and calling the method from that object. I'm honestly not too sure!

You could consider placing this section before the case for when there are no tasks in the day. Something to do with making the happy path prominent:

https://nus-cs2113-ay2021s1.github.io/website/se-book-adapted/chapters/codeQuality.html#make-the-happy-path-prominent

Should this import be removed if we are no longer using the Ui class?

Oh yes I forgot about that. My bad!

You could add the "-1" logic in this line, so you don't need to keep typing it afterwards


            noteNumber = Integer.parseInt(ui.readCommand()) - 1;

You could consider refactoring this into the Parser class, and passing the parser object into the execute method too.

Doing this could help in ensuring SRP.

Consider adding a line in between to ensure the JavaDoc standard is adhered to.


     * Parses the deleteNotes command that the user inputs.

     * 

     * @param input String containing the user's input.

Same for a few of the other JavaDoc comments!

Could consider using constants for these:


                times[INDEX_START] = start;

                times[INDEX_END] = end;

where INDEX_START = 0 and INDEX_END = 1

same for getting the "by" of the deadline as well!

Same comment as the earlier one!

Could you consider renaming this to include "Message" for consistency?


    public void showEditableParametersMessage(String type) {

This might make it less object oriented though! I'm not too sure what is the best way also though.

Can I suggest that maybe one way to do this would be to put the methods and variables in the NoteHistory and TaskHistory classes within the TaskList and NoteList class instead? This way we don't need to pass another class into every method, and instead everything is nicely contained within the TaskList and NoteList classes.

Good point, I'll change it and see how it looks!

The reason why I left it like this for now was because I found it difficult to have descriptive meaningful method names if I were to split it up.

Fixes #5

Closed by merging pull request #13

Closed in pull request #15

Closing pull request as the build tests fail.

Trying again in another branch.

Fixed in pull request #28

Fixed by pull requests #35 #36 #37 #38 #39 #40 #41

Closed by #74

Closed by pull requests #79 #81 #82 #83 #84 #90 #91 #92

Close by pull request #86

Closed by various pull requests in v2.0

Some deadlines might happen repeatedly. e.g. A weekly homework submission, or a weekly post-lecture quiz.

Just like in our current CS2113T module, the lecture quizzes might be a good example.

Date is in the incorrect format, as specified in both User Guide and help command.

As explained in the User Guide and help command, words in upper case are parameters to be supplied by the user.

As explained in User Guide, command only shows longest block of free time.

Coloured teal to indicate current timeslot. Will be updated in User Guide.

Some deadlines recur, like a weekly homework submission.

Our module's post lecture quizzes are a good example of this as they have a fixed deadline of before the next lecture.

Archived data is not made available in the program. As described in User Guide.

Data is saved automatically.

As explained in UG and help command, words in upper case are parameters to be supplied by the user

As explained in the User Guide, LifEasier accounts for some time for the user to get up and get ready.

Use the showNotes command to view notes.

The input date is also incorrect. In this case the program has flagged this out first.

As explained in the User Guide, the sleepTime command allocates one hour for the user to get up and get ready. As such, 16:00 is the intended behaviour.

This functionality is provided under the deleteTask command.

Do you think this is better here, or in the CliUserInterface class? I did do it this way in my iP, but I'm not sure this is necessarily the better way to do it.

Does this ever get executed? I thought if there's an exception it will only execute the catch block.

This is probably ok for now, but we should really do something a bit more useful to the user than just printing the stack trace, perhaps throw a custom exception.

Maybe later we should create a ZeroNoteException class that all the regular exceptions will inherit from? For now this should be fine though.

Should this really be static?

We should probably implement a custom exception for this, but it's probably fine for now...

The whole thing inside the print statement could be refactored into the TaskList class, as a toString() method or something, do you agree?

This should probably be changed in the future to include a logo for Zer0Note.

Could we just use input[1] for this?

We could also potentially add this as a field in CliCommand, do you think that might be a good idea? Since every command will have a command word.

We could have subclasses of CliCommand for commands with and without arguments, and the subclass with arguments could contain these fields to be overridden. Do you think that might work?

We should add custom exceptions for all of these, catch those in CliUserInterface, and use an inherited printErrorMessage() method or something right?

The function doesn't actually throw any exceptions though, as confirmed by the method signature, right?

I could be wrong here, I'm sure the creators of IntelliJ know more about Java than me 😛

Potentially put the title and the content on separate lines?

Should we add a setTitle() as well?

I think execute() should call the methods below as necessary, shouldn't it?

So when you want to list the notebooks, the CliUserInterface can do something like:


CliCommand command;



if(input.startsWith("List"){

    command = new List(commandArgs, noteBookList);

} 

...

command.execute();

And then in the execute method:


public void execute() {

    switch(commandArguments) {

    case "nsp":

        listNotebooks_nsp();

        break;

    ...

    }

}

Not particularly good code up here, but I think you get the idea of what I meant, right?

Do you think something like ApplicationState would be a more appropriate name, since this class encapsulates more than just the mode?

We only ever have one NotebookShelf per instance, so perhaps this should be renamed to notebookShelf instead of currentBookShelf?

This should probably be renamed to match the field name, no?

We should move this to a setter as discussed in the meeting right? Or am I remembering incorrectly?

This could potentially be replaced by throwing new exception, couldn't it?

Nice use of exceptions here 👍

Image link appears to be dead, perhaps merge first and then fix to get the final URL

Do you think this sentence could be linked a bit better to the rest of the text? As it stands, it seems a bit random to me.

I decided to not worry about exceptions too much at this stage. There is a better way to do exceptions that I want to discuss later. For now this should be ok as it causes the program to not crash.

If you feel like it sure!

Thanks, it was ~~one press of Ctrl-Alt-L~~ a lot of hard work and software engineering, yes, that's definitely what that was...

I blame IntelliJ for automatically "optimising" that

😃

Duplicate of #7

Duplicate of #8

Duplicate of #10

This should solve #18 as well.

We could pass the input arguments, as well as the notebook list, in the constructor perhaps?Sent from my Samsung Galaxy smartphone. -------- Original message --------From: Long Nguyen >notifications@github.com> Date: 11/10/20 21:48 (GMT+08:00) To: AY2021S1-CS2113T-T12-3/tp >tp@noreply.github.com> Cc: Neil Banerjee >neilb5400@gmail.com>, Comment >comment@noreply.github.com> Subject: Re: [AY2021S1-CS2113T-T12-3/tp] Add list() function in notebook mode (#42) @longngng commented on this pull request.

In src/main/java/seedu/duke/userinterface/command/List.java:

public class List extends CliCommand {

@Override public void execute() {

But the list() function needs the object as well, like listNotebooks_nsp(Notebook sampleNotebook), so I'm not sure how to code it. Also, should the List class handle input arguments like "nsp"? That doesn't seem follow the principle right?

—You are receiving this because you commented.Reply to this email directly, view it on GitHub, or unsubscribe.

I think this PR would take care of #32 and #8 as well.

Looks like there's probably some missing imports for exceptions.

Functionality of this code has been verified to be correct. I/O redirection tests have not yet been rewritten. Merging so I can fix the style to be more consistent.

I'll try to finish as much as I can by 2100 today, after which you guys can review, merge and work on it further. Please do try to follow the style, and OOP conventions, I used as much as possible for the sake of consistency. Thanks all!

We really need to rewrite the IO redirection tests...

I believe this solves #13 and #16.

Thanks for the fixes @chuckiex3! Since you requested review from everyone else too, should I wait for more people to review before merging? I think it should be fine though.

Your wish is my seedu.Duke.userinterface.Command 😃

Implemented saving for notebooks. Loading to come soon.

Loading has now been implemented! The PR is ready for review 😄

Plan to store the name of user

How do you think we should do this? Perhaps modify the Storage class to save a file like preferences.txt or something?

I suppose this can be marked as done for now?

I don't think this is particularly suited for Junit testing.

Is Storage something we should be testing with JUnit? I guess we could test the serialization and deserialization (especially with edge cases).

Ready for review and merge for now.

Marked as draft since it doesn't seem to be done quite yet.

Added Architecture Diagram

Noted in TODO near it.

Pushing this to v2.1 (the bugfix release) so we can have more time to discuss the appropriate approach.

Pushing to v2.1.

Is it perhaps better in the long run if we do it via a ArrayList?


ArrayList<String> commandHelpList = new ArrayList<String>();

commandHelpList.add(AddNoteCommand.getUsageNote());

commandHelpList.add(AddEventCommand.getUsage());

commandHelpList.add(CreateTagCommand.getUsage());

commandHelpList.add(DeleteNoteCommand.getUsage());

commandHelpList.add(DeleteEventCommand.getUsage());

commandHelpList.add(DeleteTagCommand.getUsage());

commandHelpList.add(EditNoteCommand.getUsage());

commandHelpList.add(EditEventCommand.getUsage());

commandHelpList.add(ExitCommand.getUsage());

commandHelpList.add(FindCommand.getUsage());

commandHelpList.add(ListEventCommand.getUsage());



int i = 0;

resultString = "";

for (String command : commandHelpList) {

    String color = (i % 2 == 0) ? COLOR_CYAN_STRING : COLOR_WHITE_STRING;

    i++;

    resultString += command + InterfaceManager.LS + color;

}

return resultString;

Naming issue on the getCommandUsage

Should we standardize using pointer Boolean or primitive boolean?

Similarly, perhaps we should change to boolean if the parameter is not necessary in defining generics.

Need to rename this as well as COMMAND_USAGE_NOTE

Small error on my PR that wasn't caught. Should add full stop.

Should also rename this. Apologies for the sloppy naming.

Good catch on standardizing this so it's easier to read across the board

I'm just afraid if we add more commands down the line we need human intervention to always check if the colour ordering is correct. Doesn't affect any functionality so it's not important.

Will clean up naming after 1.0

Perhaps follow standardization after Code Review 1.0, put an empty line between group of imports. Group the import by usage/type.

You should swap the two. assertEquals method arguments are expected and actual. For the sake of debugging in the future it would be better if we caught it now


        assertEquals(NOTE1_TITLE + " pinned: " + 'N', getExecutionStringInputIndex(notebook, 0));

        assertEquals(NOTE2_TITLE + " pinned: " + 'Y', getExecutionStringInputIndex(notebook, 1));


        assertEquals(NOTE1_TITLE + " pinned: " + 'N', getExecutionStringInputTitle(notebook, NOTE1_TITLE));

        assertEquals(NOTE2_TITLE + " pinned: " + 'Y', getExecutionStringInputTitle(notebook, NOTE2_TITLE));


        assertEquals(PinCommand.COMMAND_UNSUCCESSFUL_MESSAGE, getExecutionStringInputIndex(notebook, 5));

        assertEquals(PinCommand.COMMAND_UNSUCCESSFUL_MESSAGE, getExecutionStringInputIndex(notebook, 3));


        assertEquals(PinCommand.COMMAND_UNSUCCESSFUL_MESSAGE, getExecutionStringInputTitle(notebook, "Title"));

        assertEquals(PinCommand.COMMAND_UNSUCCESSFUL_MESSAGE, getExecutionStringInputTitle(notebook, "Random"));


        assertEquals(note1ExpectedOutput, getExecutionStringInputIndex(notebook, 0));

        assertEquals(note2ExpectedOutput, getExecutionStringInputIndex(notebook, 1));


        assertEquals(note1ExpectedOutput, getExecutionStringInputTitle(notebook, NOTE1_TITLE));

        assertEquals(note2ExpectedOutput, getExecutionStringInputTitle(notebook, NOTE2_TITLE));


        assertEquals(ViewNoteCommand.COMMAND_UNSUCCESSFUL_MESSAGE, getExecutionStringInputIndex(notebook, 5));

        assertEquals(ViewNoteCommand.COMMAND_UNSUCCESSFUL_MESSAGE, getExecutionStringInputIndex(notebook, 3));


        assertEquals(ViewNoteCommand.COMMAND_UNSUCCESSFUL_MESSAGE, getExecutionStringInputTitle(notebook, "Title"));

        assertEquals(ViewNoteCommand.COMMAND_UNSUCCESSFUL_MESSAGE, getExecutionStringInputTitle(notebook, "Random"));

So it's basically to convert the event back to it's input line and stored right? E.g. add-e /t Tutorial /timing 2020-10-29 13:00 /repeat weekly /reminder 1-day. Looks great!

I feel this is a great way to save and load!

Was implementing this on my branch but since it's here it's great!

Actually for reminder, it accepts the inputs as /remind 1-day 2-day 3-day. Not just /remind 1-day /remind 2-day


        eventDetails += (reminderPeriods.size() > 0) ? PrefixSyntax.PREFIX_DELIMITER + PrefixSyntax.PREFIX_REMIND  : "";

        for (String reminderPeriod : reminderPeriods) {

            eventDetails +=  " " + reminderPeriod;

        }

Actually, what would happen if reminderPeriods that is stored was null? Should have a checker here if not a empty arraylist will be given

Apologies. Automatically did it while importing through Intellij.

I was considering that there should be no issues with duplicate events as they may be for future events that are named similarly. E.g I set an event but didn't know about recurring function so I created an event for every week.

Noted on this point. Will make necessary changes.

Thanks!

Noted, will change. But it won't cause an error as JRE handles boxing and unboxing of the non-primitive types in the background.

On this point, I was following the structure of prepareAddNote and how error handling was taken care of (Level of throw or at a higher level). Can confirm again if I should just throw the exception?

My bad. Didn't pull a new copy yet with the changed error handling format. Mine had error handled at the problem level.

Sorry! I didn't merge this branch with the upstream master. Done in the next commit.

Good catch! Didn't see this

Done. Thanks!

I considered having a optional tag that allows to list by a certain search criteria, but I felt that in the interest of breadth-iterative development, this wasn't as key to developing recurring events as it is so I comment it out first.

Done already. Thanks!

Yes but in this case these are parameters not the constant so I don't think there's any coding standard violations.

Apologies, forgot to refactor the names after changing it to final.

Alright. I moved them from parser over to here because I thought it would be a central location + parser was getting very bulky already

Alright. Apologies for the mistake. Was referring to an older version of the code base for styling and didn't change this part.

I placed it here because following the rest I thought we would expect a COMMAND_UNSUCCESSFUL_MESSAGE. Can remove comment as well.

I've re-worded the comment to be clearer as to what it means so it doesn't look as weird

Ah didn't know this was possible!

Why the whitespace? The tab itself is a whitespace

I just left it in because in the future there might be need to develop looking at future reminders. But then again in the current iteration I saw no need for it / we may not develop it at all cause of limited use cases.

I guess I was unclear. The input formats should be YYYY or YYYY-MM

I used Object cause it was more type-safe. I was considering that when the timetable expands and has multi-year usage, we may have issues with recurring events and their reminders so I was implementing the equals function to be able to compare reminders. However, I revamped how the recurring events are displayed so this function actually has no more use.

Er actually theres no significant need to do this as a class anymore. I was still learning on functional programming and functional interfaces in Java and their syntaxes so I implemented this as a stop-gap measure. Will update it.

I think it's fine because even in the lecture notes they talk about assert as a error handling as well as a JUnit. It can be used for both. I implemented it here as an assert because if its not the same then there would be big problems with the entire remind functionality.

Also, if there is difference in the two, it would indicate issues in the parser or implementation of the code, nothing on the user, hence exception is a less viable option.

Was causing a bug with recurring values

I did it this way initially but my concern with handling it in the formatter is that it breaks encapsulation and increases coupling. Violates the "tell-don't-ask" principle, but handing it to the formatter improves single-responsibility principle.

What do you mean?

Oh I did it as a separate function for compatability to other PR where I do listing. No point re-writing code just to iterate the same lines.

My considerations are formatEvent would be heavily utilized when listing events. When listing multiple events we would most likely be listing them in the same box so it would make sense to have a separate function that formats them as a string.

See above regarding formatEvent

Tried this and the line got too long and unreadable.

Sorry but what's the significance of doing that as opposed to +=?

While possible, I'd lose the information on the name of the month and at each event, I'll have to check the month and see if it's different at each step. I'll basically be shifting the processing of each event from Event to here. Is there any significant reason why? If not I feel there's no added benefit to doing so.

I'll see what I can do as I am already currently pushing in the year and month already. May be able to cut down the data struct shape

Done from earlier PR

Will resolve convo when confirmed after merging.

I was thinking of doing this because it may have edited the data but failed the saving

Nope! It'll display as Reminder: None

Are we done with this issue or do we close it?

Noted. So for this we should highlight it in the developer guide right?

Able to recreate bug when doing [list-e] vs [list-e ]. Spaces after list-e causes this issue.

Okay thanks for the clarification! Closing this.

Resolved.

Resolved. Will push branch together with refactor of listEventCommand.

Was not tagged with the PR that added tags on event. #136

Should the creating of a new instance be inside this function instead?

public TeamMemberList() {

 members = new ArrayList&gt;>();

}

Should we account for zero team members? Catch an error?

Good implementation to switch between different views

Would extracting to the UI class be neater?

So everything related to printing will be under user interface.

Similar case here, would extracting the printing function to UI class be neater?

Would it be clearer if the name of the variable is changed to secondProjectIndex.

Would combining these statements into one single command be neater. Use line wrapping for the different lines of output.

I think that output-ing a warning for the user and ask them to provide a full name or have them provide a specific index would be better.

Can consider changing the exception name to something else other than Duke, unless we are still naming our program Duke. (small issue)

I feel that this part is confusing. I understand that it is supposed to throw an error, however the exception message is "add task", maybe change it to "add task error"?

similar case here, would changing it to "add project error" be clearer?

similar case here, would changing it to "switch error" be clearer?

You can check my implementation for this variable so that when we print out the description, there will be spacing in between the words.

require an else statement? We can implement the "deadline" function for projects as well.

I would be able to do that since I need to combine it with printing out the statement during the "select" function.

If approved, I will create an issue for it and start working on it.

Im not sure about the rest, but I got confused when i first read the code.

Maybe we can change the name to getTaskInsideAProject()?

same here, would changing to createTaskInsideAProject be better?

same here, would changing to deleteTaskInsideAProject be better?

same here, would changing to getNumberTaskInsideAProject be better?

Should we extract this to Ui class or Exception class??

Should we extract this to Ui class?

For future reference, should we remove this as the task does not require multiple inputs.

For future references, should we remove this??

I don't think there is a need for IndexOutOfBoundsException here.

I think we should add a try catch statement here, seems like there is a need to catch an error here if the ID provided by the user might not be within the range of the task arraylist

hmmm, you are right I can combine it together with the toString() function

Fixed!!

Violates coding standard due to *

This method seems a bit long, maybe do more abstraction

Need to remove the null due to updated constructor for DeleteCommand. 1 more occurence below.

Violates coding standard for ternary expressions

Unsure why the spaces?

Not sure about the readability of this line

Need to add a space between ) and { to prevent checkstyle violation

There are syntax errors here, see the checks tab for more info

Not sure if this is magic number?

Maybe use .isEmpty()?

I believe the command should be changed to "edit -t"

Maybe append with COMMAND_WORD here instead

Curious why javadoc deleted? 1 more occurence below

I think the COMMAND_WORD should exist in the parent class EditCommand instead

Same concern that COMMAND_WORD should exist in the parent class EditCommand instead

Now the manager is able to take values from Mon to Fri. Will attempt to update it to be more date specific.

Maybe set a variable to the set so that you do not have to keep repeating the getModulesMap()

Maybe assign a value to moduleCode.getModuleCode() so that you do not keep calling it

I think I have seen this method appear multiple times in many parsers, could we extract it to a method?

Is there a reason why this is protected rather than private?

Format for the deadline is d-M-yyyy HHmm\

e.g. add -t read chapter 1 -by 12-9-2020 1400

Agreed, because the task index can only be found through list

Maybe try to use enums instead to improve readability?

Same comment as above, I do see the explanation in the javadoc but it might be better to change to enums

Pull request is failing due to enabling of the gradle checkstyle. This means there are files that are failing the checkstyle, please check the checks tab and amend the files thanks

Thanks, added

Unintended edits

Ok, will make amends

I think the commented code can be removed. But otherwise, looks good to merge.

Amended the AddCommand, DeleteCommand, enum in Parser. Checks are failing due to the functions it requires from other classes.

AddCommand:

testDeadline(String deadline)

addTask(String desc, LocalDateTime dateTimeOfDeadline)

addModule(String desc)

execute()

DeleteCommand:

deleteTask(int taskId)

deleteModule(String moduleCode)

execute()

Looks good to merge

Looks ok, but the Parser needs more prepare commands (delete, list, done)

could u add, a AddCommand.FORMAT

a constant that outputs the correct format on how a addcommand will be read

Ok, added

Implemented on purpose, but has been amended.

There is some bug with this PR, checks are running forever.

What's the bug?

Used to be a bug with the JUnit tests, but it has been fixed.

Accidentally closed this PR, reopening

New unicode character fixes the cross and tick

Nice job on creating the package early

Yup we don't need this anymore!

Make sure to have this on two seperate lines

Brilliant

Well organised the code looks really neat!

Maybe consider renaming the "time" parameter to "startTime" to avoid confusion

Consider renaming the "time" parameter to "startTime" here too as this is the time the task should task.

Consider renaming the "Commands" class to "Command" as this relates to a single command.

Yup good naming!

This looks good!

It may be better to leave this in lowercase, as this is what our user guide says. We can convert this into an enum in our next version?

Consider renaming "c" to "command"

Renaming this to taskList.addTask would be great 😃

Good that the importance is being converted to upper case here!

Remember that the user will type "high" instead of "HIGH"

I think we can remove the first addTask() . The method right below this seems to have the same functionality in addition to assigning a unique index value

Noted that the user will now see the list from the timetable class only

Looks great!

This test will return true once 23-10-2020 is in the same week as the current date. Perhaps there's a better way to generate a false case. Maybe the recurrence can be (LocalDate.now() + 7 days).toString()

This test is exactly the same as testIsTaskIncluded_filterByToday_returnsTrue(). Perhaps a better input could be a day that lies in the same week as LocalDate.now and adding this as a parameter.

Good idea to simplify the code

Will we have to change the dates EXPECTED.TXT everytime we test?

Remember to update this new default value in user guide

Seems like you have a getDurationString() it may be easier to use that?

Noted! Has been changed to edit

@amanda-chua take note method name has been changed to addTask

Whoops forgot to remove this

ooh good spot

I just implemented this for debugging purposes I have removed it now

Whoops forgot to delete

Yup let's change it to logicManager once that's merged

Oh whoops

Okay I'll delete this and add it in another branch

This gives me a syntax error for some reason

Oh hmmm I thought it may be better to include the actor for the parser?

Yup for isMidnightClash the algo basically checks if both the end times are less than their respective start times. If so, both tasks go past 12am which means there's a clash at the 12am mark.

As in ATHENA still alows task to be added provided they end at 12am but not past that

okayy thanks! will make the change

Left some comments about some parts of the code, some are just suggestions, the important thing is the main function inside ForecastFilter. I think you forgot to remove it?

Also, can you add Junit test cases for ForecastFilter? Or since we are down on time we can also merge this pr first and add that later.

Noted, will add Junit test cases for ForecastFilter tomorrow

Junit test cases are now created for ForecastFilter

Note recurrence will still be "today", I have just created a recurrenceDate attribute. This is to make it easier future versions when recurrence will be a day of the week, so it'll be good to leave that as a string.

Supposed to be like this. Other members please confirm and close this issue.

I believe this can be done using r/DATE where DATE is any date you intend to add the task.

I believe this is actually #200. The assignments are being successfully added as ATHENA mentions but there may be a prior invalid recurrence.

This issue doesn't seem to exist on my side. Other members please help to confirm

I think we should leave the start ID as 0? As it is a task ID not really a number in a list. Other members, what do y'all think?

I think this part may throw an exception when the user doesn't enter an integer input.

Otherwise I think it looks good to merge 👍

I only used this line to check if the user entered valid arguments but can be removed for your part of the code if needed.

Same for this as other comment.

Typo here

I was thinking that if we only print a new sorted list using the "list" command, the users will never know which task they are setting as "done" or "deleted".

Yea that makes sense.

I was wondering if these fields should be part of Item or they should be part of Task because not all items we're adding might have these fields? For example if new classes like Expenses extends Item then it may not need all these fields, or let the fields be set.

Similarly not every item is going to save in the same format so maybe it might be better to override this in each subclass instead of putting them in the Item superclass.

Maybe having the fields in Item is okay, but it's to put the getters/setters in Tasks or related subclasses so not every Item has access to it?

This file can be deleted since it was added to .gitignore

Maybe it will be better to move this to a new method called CommandCreator.parseAddCommand that returns a Command. Then put the rest of these code inside the method that will call either of the two createAddCommands.

Consider removing this file because other users may not use it (add to .gitignore)

Maybe it would be better if "isReturn" and "futureDate" is in Book.java (not a feature of the Item but of Book)

Maybe all the book related functionality should be in Book

Same as previous

Same

Maybe BookList related functionality should be in BookList

Same

Yup I think it LGTM except that Book related functionality should be moved from Item to Book and same for BookList related functionality from Item.

what's this

maybe this whitespace isn't needed

Maybe it's fine because it finds the nearest wednesday so that the user can just enter a date range without worrying about the actual starting date.

Yea sure

Add EXPECTED.TXT and input.txt IO redirection tests from the iP

oops checkstyle failed

Adding a task

Format: add TASK_NAME [by/DD-MM-YYYY-HH:MM] [at/DD-MM-YYYY-HH:MM] [p/PRIORITY_NUMBER] [class/CLASSIFICATION]

Adds a task with a given task name.

Example: add tP meeting,

add tP meeting by/16-09-23:59 at/15-09-2020-11:00,

add tP meeting by/16-09-23:59 at/15-09-2020-11:00 p/1

add tP meeting by/16-09-23:59 at/15-09-2020-11:00 p/1 class/project

Expected output:

Noted. I’ve added this task.

tP meeting by: 16-09-2020-23:59 @ 15-09-2020-11:00

LGTM

LGTM!

It seems to be failing I/O redirection tests, try modifying EXPECTED.TXT with the new contents of ACTUAL.TXT and verify if it's the intended behaviour.

Might have to change input.txt too since some of the commands are changed.

Consider refactoring creating the command as a new method in CommandCreator class using the design patterns. You can pass in the full command or parsed command as an argument for the method. Check the add command and set command for an example. I didn't move everyone's commands because I didn't want to claim credit for the code.

There's so many constructors to ListCommand now, I wonder if we can actually pass in an enum to the ListCommand instead and use a switch statement in the execute method. But that will be for another time I guess (maybe v2.1 or another PR sometime later).

But then with enums we'd have to pass in blank arguments for "category" when listing category, so another point to consider.

Other than the minor refactoring I think its LGTM!

This should be ready to merge.

Can add the sequence diagram to the DG

LGTM!

There is no c/ tag here. It is explicitly stated in the UG that optional arguments take the form of >key>/>argument>. There is no argument, hence the "c/" forms part of the description for the task.

However I will make sure that / does not appear in the description and make sure it throws a DukeException.

Hi, actually if you do not input a priority, the default value of any task will be set as p:0. We will include this in the User guide so it will be clearer for users.

Yes, the form of an argument is stated to be >key>/>argument>, thus p/ is treated to be part of the description, as there is no >argument>.

It would be better to change to getProductName();

I think it is okay to put it here since this comment is command-specific.

I think it is okay to call it here since it is command-specific

It would be better to mention specifically what are being imported instead of using wildcard *

Good job on coming up with the sort algorithm, Kian En! I would move this to AddPlaceCommand.java so that it sort only if the user added a new place.

It supposed to be userInput.startsWith("search "). If not, input like searchex will be detected as valid command.

Yep, this has been resolved.

It is preferred to mention explicitly which packages are imported

What is the difference between this and line 33?

No newline here

This supposed to be rmdir /s /q trippie_data

del is used to delete file. Based on runtest.sh looks like you wanted to delete a directory and its content, right? Use rmdir /q /s trippie_data instead, it is equivalent to rm -rf

Since your PC OS is macOS and mine is Windows (Error is in .bat file which is executable in Windows). I will take-over this error.

Isn't this deleteExpenseCommand?

Add newline here

Another newline here

An alternative way is to use .isEmpty() method

Please add newline here

Perhaps deleting this method as we aren't going to perform any test?

Great implementation of regex

Good, this way the sorting algorithm will terminate early 😄

Good job in completing the testing guide!

~~| is bitwise or, can you explain why you choose this approach?~~ Sorry, after checking it, I think both of the approaches will lead to same result 😄

This is just a test

Please review #22

Apparently Java has a problem with a dash. So, I suggest implementing / (slash) instead of - (dash) for the command format. @ShawnTanzc need to adjust accordingly.

Apparently Java has a problem with a dash. So, I suggest implementing / (slash) instead of - (dash) for the command format. @ShawnTanzc need to adjust accordingly.

It doesn't pass Gradle check

remember to update the help stuff

Do you mean on the HelpCommand.java?

This is fixed in #75, right?

Good Job!

I think a better naming for this would be "questions"

I think we should all following this naming convention of "all...".

All these naming makes it easier to understand!

The JUnit test looks good!

The naming of the method is accurate as well.

Would "chosenOption" be a better way to name it?

Nice, using the common interface makes everything easier and neater!

I'm a bit confused with this. It's a add method but it removes the option...

Nice catch on the accessibility

Maybe we could add something here for users who had wrote commands not in our list. I can create something in Ui

Awesome refactoring!

Just some minor details, would the first part of the naming be better to follow the naming in Topic? getDescription_... Similar to getsQuestionList_questionList_returnsQuestionList( )

Oh thank you for adjusting the formatting!

Oh sorry I missed this out!

Oh, it sounds a lot nicer!

Nice catch! Sorry for that error!

Would it be better if we have a few words to introduce the diagram first?

Nice catch on removing the empty constructor too!

Ah this makes it look a lot cleaner and easier to understand! Thanks!

Thank you for doing up the table of contents! It looks neater and easier to understand now!

Thanks for removing the magic literals!

Nice catch!

This makes it a lot clearer!

Ok, I will take note of this and change them. Thanks for the explanation!

👍

Ahh, slipped my mind! Thank you for pointing it out!

Ok done!

Ok, will look out for it. Thank you!

Okie!

Oh ok, I think it's 4! Thank you for spotting it!

Okie, I will look into this. Thank you!

Updated! Thank you!

Oh ok, I have changed this too! Thank you!

We have decided that the users can just change the database via the Json files

Fixes #119

The user guide has already been updated.

User guide has been updated

perhaps remove some of the old Duke descriptions

perhaps a more apt name than executeFunction(), idk

maybe remove the spacing between first line of the method and the next, for consistency

er like the line i commented on said duke.txt and list of tasks

i'll probably try to move this to cheat sheet class

Not really sure about the use of super here since DirectoryIsEmptyException didn't do it

to keep consistent with other if statements, might wanna do a .trim() here

not sure if line 62 to 67 is needed. line 65-67 seems like it can go into 69.5

A few of the class names sound more like method names. Perhaps instead of ParseDataFIle, DataFileParser

perhaps parse execution can be renamed it took me a while to understand 94 - 97. perhaps it could be renamed to parsedData or something

it seems like for both read and write classes will create Files if not present already, perhaps make It only the job of write to create both the folders and files

perhaps this could be aptly named as my understanding is that it points to an actual file and not a directory.

clean code makes me go yes

good method namings

big brain

probably can use getCheatSheet(name) instead

I'm not really sure if they would be nitpicky on the use of '*', I've been trying to avoid it because of that.

Code looks clean, LGTM!

i think so but it's really ugly and I hate it

perhaps cheatSheetList can be moved to the parent class Command

alright fixed!

alright! done

Alright done!

Ops i merged forgetting to check the tests LOL Brandon has fixed the tests

Done for view command

alright, will do

For this i will be parsing in an integer alr so i think can remove the getTaskNumber method.

Can replace all the println to UI methods.

Remove one empty line from here.

Remove empty lines from here.

Remove one empty line from here.

Change the importance to notes

Remove extra line

add space before String

remove the empty lines

remove the empty line

I think taskListWithoutTask should be editedTaskList

I think taskListWithoutTask should be taskListWithDone

Change taskListWithoutTask to taskListWithDone

Might want to change the method name to smt like deleteTask_successfully?

Maybe can give examples of ABC

List out the commands individually instead of the *

For add command: throw exception when name or time of task is not in the user input.

#225

good job!

Ah yes, this makes sense. We can cut down on unnecessary variables this way!

The DisplayableList class seems a bit redundant at the moment as its functions are very similar to an actual ArrayList. We should look into developing this class more to better manipulate the lists in our program.

The coding logic here looks really good! Nice work!

Since you're getting QuizQuestionList with this function, would calling the new QuizQuestionsManager object quizQuestionList instead of quizQuestionsManager?

The diagrams all look really good! Nice.

Looks really good, the explanation is methodical, while using concise language. Nice job!

Thank you for helping me replace my magic literals as well! All of it looks good to me. Will merge it in!!

Nice, you remembered to use System.lineSeparator() instead of "\n"!

Good thinking, this interface will allow us to differentiate functionality more cleanly between certain lists that are purely for display/access, vs others that the users might wish to affect. Nice implementation!

Nice removal of magic literals.

LGTM, now bookmarks can be saved across multiple uses of the program.

Looks way neater, nice!

This looks really good! Thank you for settling this!!

Well implemented!

Nice, this covers a lot of ground.

Alot of coverage here as well, great implementation of the Junit test!

Good point! Will make the recommended alteration right away.

Ah yes, pardon my folly, will adjust it in future. Thank you!

It was initialised above the if loop.

Alright, noted! Will look into how to make the implementation better in future!

Roger, this makes a lot of sense. Thank you!

Understood! And done!!

Overall, well done changing your name.

I think it would be better if you change this to plural form since it contains a list of members. Likewise for the class name.

I agree, we should split them into subclasses, inheriting MemberCommand for example. It's what the TA suggested.

Yeap, Haoxiang, remove this file then commit again to the same branch, then we will merge this pull request

I think it is ok, we can leave it as it is now and we will settle the package names later on

I think wait for the next person to review then merge.

What's the reason for using static members? This will be a problem when it comes to multi-project management.

"member" can be changed to "members" which will make more sense as it is a list of members.

Yeah. Since it is not static, you still need to pass the project object around to be manipulated, and that should be the way since there will be multi-project management in v2. This is because we need to determine which exact project we are manipulating.

No need for new Hashtable>>(); since you are overwriting it at line 15.

Should we trim the values at Parser instead?

Sorry, missed this out, this can be SLAP-ed right? So that REMOVETASK and other actions that requires array list can use the same function.

Alright

I think no need to nest another try-catch, you can just put another catch for NumberFormatException in the other try-catch.

Got a getter for nextId already, at line 37

Can you remove this line? This is because line 34 has set exit = true. In Duke.java line 63, I have already added the check to check if the exit = true. Also, the Duke.java has to call sm.save() before it exits.

The code analysis stated this line don't need StringBuilder since you are only appending on string. you might wanna change that.

The newline should also use %n instead of \n. %n works similarly like the System.lineSeparator().

@gmit22 Can I double check with you if this is supposed to be == or !=?

This PR added a shouldSave boolean variable to all command classes. All command classes are required to define the shouldSave value and I have edited all the command classes to define it. Basically, those that cause changes to the data will set shouldSave to true. If you look at Duke.java, it checks the shouldSave value of the command class that has executed and invokes the save if shouldSave is true.

In your case, the project should have been successfully created and the save happens before the crash. That is why it shows the project that was created in the first place.

Yeap

Functionalities I think are good to go but I think some variable names should be changed. For example like those that contains a list of elements, the variable name should be plural.

Looks good to me, can merge imo.

Update: JSON.simple library is already approved

Looks good to me, I think can be merged.

checkStyle is failing. You should fix them first before you make final commit.

Good to merge. However, we need more discussion of the structure of Ui class because I feel it is too verbose. Maybe we can proceed with the commit for now then modify it later on.

Looks good, can be merged

Need to resolve the conflict first.

Duplicated issue

There's conflict that needs to be resolved.

Have you tried enabling the assertions? Link

You are talking about the Unit Test assertions is it? Those assertions are different from the normal assertions. Normal assertions are placed in the code using the assert keyword, therefore the link I have provided only works for normal assertions. Anyway both have different purpose, so yeah.

I think we can merge this one, then you create another PR request for the JUnit.

Looks good to merge.

Thanks!

Don't merge this first, idk why got duplicated checks... Probably screwed up with the repo just now.

From what I see in your branch's Project.java, it might be the case that toJson did not include SprintRetrospectiveManager data. Hence when the save happens, nothing is saved for SprintRetrospectiveManager and its sub-objects. So when you reload the program and try to access it, it is empty. You can verify this by inspecting data/data.json file to see whether the data for SprintRetrospectiveManager is inside.

You can refer to how I did it for SprintManager, ProjectMembers and TaskManager. They should be similar to your class.

Can call "ui.printList(ArrayList>Task> tasks)" and let the ui handle the printing

Can call the printList() function from ui and pass it the filtered list (so tasklist will still handle the filtering)

Could call ui method printTaskEdited(task) to print out the edited task to the user

Could call ui method printTaskDeleted(task) to print out the deleted task here as well

Can call ui method printTaskAdded(task) to let users know what task they added

Can call the ui method printTaskDone(task) to let users know which task they marked as done

@alstontham The timetable given to the Ui will be a list of TimetableDay objects, each containing a date and a list of Tasks. So you can work on printing that now.

Ok! Will work on this

@alstontham I think it would be good to overload the toString method so that we can print the object directly. You can work on printing out a Date and TaskList on your side first then we can move that code here later.

Or you can fetch this branch and modify it, then make a pull request to my branch.

Sure, will see what I can do

would checkstyle flag this extra space?

I'm not sure if the main method is supposed to throw the exception? The exception should be handled at a "lower" method

Maybe a more descriptive method name would be helpful?

Can remove extra spaces here

Do you want to shift the methods that print out the Timetable to Ui?

Could you add a "\n" at the end to make the spacing of the Ui outputs consistent?

Same here, add "\n" to making spacing of outputs consistent

Hmm not sure why it shows up like this on github but I definitely had it on two separate lines in my IDE

oops my bad

Ok, I'll amend it

will fix this

oh this wasn't in my commit but was accidentally added onto this PR

I'll change it to AddMissingRequiredParametersException

This exception isn't finalized yet as I need to wait for the edit command to be fixed so I can test it.

fixed!

Fixed the checkstyle issues in Ui.

Resolved in #55

I have added exceptions to handle when no index and an invalid index has been passed to delete in #48

Yes, this PR partially addresses #73, however I have yet to properly implement exceptions for list and edit as I need to wait for those commands to be fixed first.

Sorry my bad, I thought I changed this in a previous commit but I think it got changed back.

I refactored the color and UI code in this PR so it can be compared with the UI information in the DG

I haven't finished editing the DG yet, will push some more commits soon!

Square brackets around t/TIME, add view command in the help method

I added an if statement to check if there is no deadline and prints the message accordingly. Addresses #159

Make the instructions in the UG more clear.

Same as #207

Could have a final static for this magic number

alist should be in camelCase

adatetime should be in camelCase

this comment could be added into the ui

can refactor this into ExceptionMessages class

magic number here, perhaps can make it as a final variable

magic number present here

naming should be InitialiseUser instead of Initialiseuser

the naming of this should be activityMultiple instead

presence of magic numbers

perhaps naming of firstString and secondString would be more appropriate?

these variables should be in caps

this should be in caps

perhaps a better naming could be done for this?

same issue for this, perhaps a better naming could be done for this?

noted

HAHAHA oops thanks

fixes #41 , #27 , #45

fixes #18 , #19 , #21 , #22 , #23

fixes #88

A very minor thing but I think you missed a whitespace after the ':' in DETAILS.

Whoa Theo god used assertions alrd 👍

I think can change to something like this for better readability


cheatSheetFileBuild

        .append(DETAILS)

        .append(WHITESPACE)

        .append(cheatSheet.getCheatSheetDetails());

Isn't it better to just write the stuffs in storeCheatSheet() inside executeFunction() directly?

Why do you replace the string with whitespace " " instead of an empty string "" (?)

Will work on it 👍

👍

Actually for this one, do you want me to add a method to remove cheatsheet by index in addition to the current remove by name?

For the next iteration, I think can move this to UI class. Not urgent tho

I think this one should be done by the constructor, or at least outside the for loop.

If can't find programmingLanguage and keyword, I think can just print everything (coz no filter?). What do you think?

hmm for this one I think just do it similar to our IP? create a variable inside a for loop in print method then just print the variable?

wow you guys so advanced.. using hashmap and stuff 💯

Should we add .trim() to avoid this?

shall we change this to /s?

Noticed this one is duplicated from getIsFavourite

Currently when I tried

/delete /i 2

The description map will return


NAME -> NULL

INDEX -> 2

So without the null check, it will always try to get the cheatSheet by name even when the user tries to get it by its index

I think the variable name userInput is better than command.

A better exception name would be BadFormatException.

This needs to be called from Ui as well, once its implemented.

userInput will be a better variable name than command.

This shouldve been an abstract class for all the other commands.

I think creating this class as a parent for everything else will solve the parser error.

commands should be appended with a space character.

Why is there a need for this return statement?

Nice formatting, LGTM!

You can use Java's smart for loop for this

Is there better naming for without choice?

Good use of bubble sort optimization.

This will still allow empty or numerical currency abbreviation.

Can use regex to make sure it only contains uppercase alphabets.

this works as well though. Nice!

can be simplified using:


input.equals(input.toUpperCase())

Unlike read command, this line is used without printing any symbols before the user input.

Umm.. did I remove the lines by accident?

shawn I think there are some of your changes I accidentally deleted. It's the automerge..

okk

No, this was changed in this commit. But we could think of this feature again. Link to commit.

Thank you @felixhalim

I tried to delete a directory, and output no errors if it does not exist.

Simplified the if-else statement.

Simple O(n) linear search, should not affect performance.

Duplicate issue

Hello Sameer,

I opened a new issue for the mismatched project requirements. Our team will update it ASAP. Please check #19

Regards,

Ivander

Todo: Fix runtest.bat (@felixhalim )

Thank you felix!

LGTM! Nice.

Fixed in #18

This MaxDay is integrated with the storage component. It will load the max day in the master file. Fixed this issue.

Thank you felix for referencing the issue.

Fix #79


    private static final String[] AA1 = {

        "PGP",

        "Kent Ridge MRT Station",

        "Opp University Health Center",

        "Yusof Ishak House",

        "Central Library",

        "Kent Ridge",

        "Museum",

        "University Town",

        "University Health Centre",

        "Opp Kent RIdge MRT station",

        "PGPR"

    };



    private static final String[] AA2 = {

        "PGP",

        "Kent Ridge MRT Station",

        "Opp University Health Centre",

        "University Town",

        "Raffles Hall",

        "Kent Vale",

        "EA",

        "NUS IT",

        "University Health Centre",

        "Opp Kent Ridge MRT Station",

        "PGPR"

    };


            assert !locations[0].isempty() : "Location 0 is empty.";

            assert !locations[1].isempty(): "Location 1 is empty.";

Looks good. Can consider using .isblank()


There are 7 commands you can use to get bus information from Nav@NUS



>Notes about command format:

Would be good to start each section with a brief intro


This following steps is a guide to start running Nav@NuUS.



1. Ensure that you have Java `11` or above installed in your computer.

Would be good to start each section with a brief intro

Perhaps we should add getters for faculty and info here too. Otherwise we can go ahead and merge

Sure, we can add it when implementing the other features

what is this for?

Why use setUserInput instead of making new Parser object?

I think it'd be good to refactor the lol to final dummy parameter for the sake of readability.


        parser = new Parser(DUMMY_PARAM);


     public static final String DUMMY_PARAM = "lol";

     */

I've removed this file. Wasn't meant to commit it to branch

Good point!

I think we can leave such prompts out of the output of commands?

Hmm then should we implement this for projects and tasks also?

It is task view iirc. isProjectListView refers to project view so means not in task view

yup agreed

Apparently, it can't catch NullPointerException. Think you could work around it?

I think the '2' is the limit for integers so in that sense I believe Shreyas' concern is valid.

I have 2 suggestions:

We loop through the inputs and extract out key-map info for multiple params
We only include 1 param type for all our commands

Ok got it thanks for the clarification

I believe this is not recommended (importing with *). You can just revert back to the old implementation

Do we need this....?

Don't need this here if it is already caught in parse(). We try not to repeat codes but do let me know if there are other reasons for this implementation

just need to include the teamMembers ArrayList in signature

Might have merge conflicts since "assign" is also used for project view

We have not abstracted the HomeView for other commands so maybe leave the HomeView in the Parser?

do you wanna change it to if-else? Just a suggestion for better readability but this works too!

Yup have added a remark to throw error

I think it's just for good practice

true, have removed in my next commit

Oh thanks for the review, just committed the changes in Home Command PR!

Solved!

They haven't implemented it yet so I'm not sure how to link it but yes that's the intention

actually in the parse function there will be nothing to catch.

If you realise the only thing the parse function does is extract the user input.

It does not process it, so no errors thrown

I think as of now that feature is not in our user guide. Like showing the list of tasks attached to members.

So since no other command uses it for now I didn't implement it.

It's a breadth first approach

I think as of now that feature is not in our user guide. Like showing the list of tasks attached to members.

So since no other command uses it for now I didn't implement it.

It's a breadth first approach

I have no preference, I think both make when it comes to organisation

I thought of it also but I realized it's more of an improvement since as of now there isn't a need for it.

We have no feature planned that will use the enums, (e.g sort by priority).

Also, the error handling is quite tedious to implement if we use enums.

I will implement it in the final run once we're done with a working 2.0.

I need help with jUnit test for this feature.

I'm unsure about how I should test invalid project ID and deadline format since the exceptions are thrown inside Parser class and not executeCommand().

What do you mean? For JUnit tests you don't need exceptions tho. You can hardcode the error messages.

Check my JUnit tests

I think this printSubject function should be implemented in Ui class instead of here right?

Should have created new exception classes for flashcards. NoSubjectException is supposed to be used by subject commands, but I noticed that the topic commands were also using the same exceptions, so I guess we can change it later

List of flashcards is not used, can be removed

This method is no longer needed. I will delete it in the subsequent pr.

Inside the constructor of the classes that extends Exception, it should call super(message). In this case, it should call super(printNoFlashcardsError).

Personally I don't think it's a good idea to make Ui to return string the exception message in this way if no cuztomization is to be done to the message. Constants would be better in this case.

We could create a method in Ui such as printError(Throwable err) and pass the error to that method for it to print out. In that way we let Ui handle all the printing and improves cohesion.

For headings such as 1. Introduction, there should be a fullstop after the number iirc. For subheadings such as 4.1 Subjects they are not needed.

Should have a full stop after 2, same for other headings

imo note instead of warning would be better here, since nothing bad will happen even if they add extra spaces in this case

Same thing as above

The end of the activation bar looks kinda incomplete for both diagrams. In the second image the parse() call to TopicParser seems incomplete as well.

I think the rightmost part is cut off.

necessary instances such as storage can also be passed in if they are needed in certain execute methods.

Issue is solved. Closing this issue.

partially fixes #29

Partially fixes #107

The results.json are not duplicates, but rather results under subject or topic. I will change the file name to differentiate between the two to avoid confusion, and update the documentation to make it clear.

4 spaces instead of 8


    private static ArrayList<Bus> buses = new ArrayList<>();

Might be better to separate i to a counter and a boolean for better clarity.

Good test cases

Nice! The tests account for most situations.


            if (info.name.equalsIgnoreCase(input)) {

usage of getName function is redundant.

Good test cases for the program

might be better to move this to UI


    public static Bus selectBus(String busCode) {

Is it possible to use an assert here instead of try-catch since the catch block is not doing anything? I'm not very sure

Why is toLowerCase removed?

ahh ok


package seedu.duke;



import java.util.EnumSet;



public enum BusStops {

    PGP("PGP","prince george residence"),

    KENTRIDGEMRTSTATION("Kent Ridge MRT Station","mrt"),

    OPPUNIVERSITYHEALTHCENTRE("Opp University Health Centre","opposite UHC, where students can get MC"),

    YUSOFISHAKHOUSE("Yusof Ishak House", "Location with student service center and restaurants"),

    CENTRALLIBRARY("Central Library","also known as CLB"),

    KENTRIDGE("Kent Ridge","idk"),

    MUSEUM("Museum", "Opposite Faculty of Engineering"),

    UNIVERSITYTOWN("University Town","place full of fun"),

    UNIVERSITYHEALTHCENTRE("University Health Centre","UHC, where students can get MC"),

    OPPKENTRIDGEMRTSTATION("OppKentRidgeMRTstation","MRT"),

    RAFFLESHALL("Raffles Hall","Hall located nearby Faculty of Engineering"),

    KENTVALE("Kent Vale","somewhere"),

    EA("EA","located in Faculty of Engineering"),

    NUSIT("NUS IT","Opposite Central library"),

    PGPR("PGPR","prince george residence");



    private final String description;

    private final String name;



    BusStops(String name,String description) {

        this.description = description;

        this.name = name;

    }



    public String getDescription() {

        return description;

    }



    public String getName() {

        return name;

    }



    public static void listStops() {

        for (BusStops info : EnumSet.allOf(BusStops.class)) {

            System.out.println(info.name + " : " + info.description);

        }

    }

}

Yup, only for testing.

sure why not

because I need the previous input to save into fav, new parser cannot save the old input

it is just a random initialiser because you need a string within the parser to initialise it

Better skeleton bus data

we might have to implement our previous idea (isValid) @Lezn0 your thoughts?

sure.

He haven't start on it. He created the classes as a template first I think.

AddNoteCommand.getCommandUsageNote() change to AddEventCommand.getCommandUsage()?


                + AddNoteCommand.getCommandUsage() + InterfaceManager.LS + COLOR_CYAN_STRING


    private static final String COMMAND_USAGE = COMMAND_WORD + ": Adds a note to notebook. Parameters: "


        return COMMAND_USAGE;

Redundant space?


        /**

          * Based on user inputted tags, will store the respective values in an ArrayList

          * E.g. if user input 2 tags, CS2113 and important, will have 2 ArrayList

          * 1 for the values corresponding to CS2113 and the other for important tag

          */

oops messed up the block comment.

Another space

Maybe can give a short comment on what's happening here.

@param?

@param here too

Forget to remove? Or just ignore for next PR use.

Already informed beforehand

Dont need. I have catch possible errors before it adds.

Removed throws comment

Good idea. Shall implement it right away. Thank you for your suggestion.

isnt final.

@Chongjx Take note. Cuz it was giving me an empty space when there are no tags

Junit needs input due to our plan to put get input content to AddNoteCommand class. I cannot simulate a scanner input hence I getContent first. If it is blank, user puts an input. If its not blank, it means it is from the junit class which already has the content already pre input into the note.

will do it once ive merge this in. have yet to pull the new one. might have conflicts

as of now no

I made it loop till there is content in the input.

deleteNote(index) does not return boolean, so I made it return true. Also, I cannot remove the exception, when it getNotes it will return an exception when the index is out of bounds.

I didnt realise this. I followed the format from addressbook to make it tidy and easy on the eyes. Jx and I used whitespaces when implementing the classes in the first place. I think its a minor matter as long its readable.

wouldnt it makes sense to break all of the note stuff in the formatter? and just pass the success parameter? since we are passing in the note too

Im not the author for this function. Ill make the changes.

it just means that the contents for that line is missing

its not a line number. fixed it

I know why already. the ide by default turns on whose whitespaces when refactoring

Please reject this pull request, thank you

It is now ready for review.

Need help with the Gradle checks. I suspect is something to do with EXPECTED.txt

Had to remove "help" from the ui-test. Wait till @brandonywl adds in his AddEvent and DeleteEvent then can add the "help" back into the input. Issue fixed!

Created PR first. Not ready to be reviewed. Will notify.

@prachi I've helped you added some codes for pinCommand but its commented out due to your junit test failing. The codes are for the formatting that @Chongjx has done.

Its fixed on my side. I havent commited.

Settled on adding PREFIX_ARCHIVE case

Thanks for refactoring out the methods!

Perhaps this could be replaced with a switch statement if you feel it would be neater.

This part could be changed to:

Iterator iterator = jsonList.iterator();

To remove the depreciated / unchecked warning

Following the above comment, this line could be split into these 2 parts to:

        //Advance the Iterator

        jsonObject = (JSONObject) iterator.next();

        jsonObject = (JSONObject) jsonObject.get("data");

Following the above comment to remove the warning. This chunk of code can be replaced with:

        for (Object genre : jsonGenre) {

            animeGenre.add((String) genre);

        }

Thanks for making this part neater.

Would this be better if it was refactored using the utility methods available in CommandParser?

Is this subsection needed? I wanted to do one for mine as well, but it seems weird without another subsection or "proposed implementation" section

Thanks for helping me fix the formatting for my images!

Hmm okay got it! I'll make modifications to my portion in my next PR. Thank you.

It seems that there is a mistake in the numbering here

Thanks for implementing the badges!

Ah, I left it there mistakenly. I have fixed it, thank you for pointing it out.

I have extracted the searchType as a constant!

I have extracted out the values as constants.

I have added a newline here.

I have added a newline here as well, Thank you!

Got it! I have rectified the issue. Thank you

I have made the changes, in all affected areas. Thank you for pointing it out.

Hi, thank you for this bug report. I have rectified the issue in the above PR.

Hi, thank you for this bug report! I have rectified the issue in the above PR.

Hi, thank you for spotting this error. I have rectified the issue in the PR mentioned above.

SO do we insert the code for adding and editting to project member list here?

Yes so something like project member which will be handling all this?

Good job, as @chocomango said we need to break commands into subclasses I think.

I think here the main implementation is for parser to be used for your testing?

Yes that makes more sense for setting it. Thanks

change this get to getTask, so it becomes backlog.getTask(id -1 )

Similar to add, maybe implement a removeTask also in the backlog task class

Think remove it then

Is the storage now taking care of all the details and storing it if the program crashes.

Like for example I create a project with a valid command

eg: project /create -title try -desc create a trial project -sd 5 -duration 30 and then due to some reason it stops working and crashes due to an error. The next time I run the program, and put the command project /view will it show the project created in the first place

Okay, so every time something new is added to save, it will update the json file, am i correct?

Yes i forgot to incorporate that change will do

Agreed.

Okay so just public variable will be used here? Then won't we have to pass the project object in all the functions?

Just to clarify:

So once project is created and thrown, the command returns the proj variable which is stored in the parser to edit?

>img width="243" alt="Capture" src="https://user-images.githubusercontent.com/59180102/97679876-db934380-1abb-11eb-99bd-bc375d47b1c1.PNG">

Good to merge.

I think you'll have to pass the project since all of it will be added to the backlog attribute of it. I have implememnted the add and delete, can check from there also if needed.

Code has been changes, the parameters and all so I think you need to adjust to that. So i think need to factor that in

Suggested some changes, rest looks good to merge

Please assign appropriate labels, and everything.

Any reason for moving project in a different package?

Like add all classes which need to be stored in one package?

The structure is making it a bit confusing imo, maybe change that?

Good to merge

I meant like add all the files whose data has to be stored in the same main package. Also, can you see Duke class and remove your parser?

Done by #34

Sounds good

Resolve the conflicts, youd have to do it using cli in your own repo @e0426051

Some checks do not seem to pass

I created a flag to print this if the user input is valid, and the required message The details of the tasks are as follows: is printed for the first task.

Creating an additional check of :

``if (sd==0)

 { throw new DukeException("Sprint Duration cannot be zero")}

Made changes so that:


command>task /view 0

The following task ID doesn't exist in backlog.

Please enter a valid ID.

command>task /view 100

The following task ID doesn't exist in backlog.

Please enter a valid ID.

This was done by using an additional conditional check :

if (backlogId <= proj.getProjectBacklog().getNextId() && backlogId > 0)

inputs[1]

this naming brings clarity! good job sam

why printing and returning a string? doesnt the return string from executeCommand in every command get printed?

nice :^)

nice job on including the id here shreyas

is there a reason why we need this?

nice job in initialising!

we will need to change this to "n" soon!

nice job on this sean! thanks so much

oh sean, this was added for the Project View. but its ok! i can modify project view to take care of this case when the priority is not set

in summary; no changes required for this on your end

dont need cause the variable is already initialised on this line

thanks for pointing that out sam ! fixed here 7cd85a2268c4e2eeb93a769ebcb82c17dd9f30f9

ok! thanks man

thanks sean!

can extract this portion into its own function

refine this comment

@riazaham the else block would be blank though right?

progress as of 19 october 2020, 7:30 pm

moving on to extend task functionality first

Hi Sean @thatseant , let me answer the points:

There's no way to display project view other than selecting project.

Is there a need to display project view other than selecting the project?

Multiple members are not shown.

Currently, each task can be assigned on member.

Tasks are not numbered so it is hard for user to see index of tasks.

Oh okay! I have built this from the mock-up that we approved and I did not include the index at that point. Maybe for the next iteration we can add an index column? For now, users can use the list command?

There is commandParams in CliCommand class. Could it be better to just utilise them?

Overall well done! I like the detail you put in to splitting all the methods.

Is it better to phrase this as "Switches from notebook mode (at the bookshelf level) to the timetable mode or vice versa:. Since there are no other modes currently.

Missing expected output

The application will not return an error for having spaces. I'm not sure whether that was intended or missed restriction.

Should be tag 1 /tschoolWork

Should be "tag /tschoolWork"

Extra new line here

>br> probably redundant

Should be diagrams/class/jpeg/Storage_UML_class.jpg

Section 4.3.2 done

Good Job.

👍🏻

You should rename your test function name in the following format!

methodUnderTest_inputGiven_ExpectedOutput

Your error: java.lang.AssertionError: size should not be greater than card index at this step

I think you meant assert flashcardDeck.size() >= cardIndex

good job !

You can look at my RemoveLinkCommand class evaluateLinkNumber to add the exceptions!

Nice !

nice dg

can change to ##Academic Component!

nice

looks good. Maybe you can rename check grade to check cap instead of check grade and list grade.

Maybe you can add a Flashcard Ui class to print out all your Ui.

Maybe you can add a constant for all your magic number to improve code quality.

Thanks for the reminder!

Okay Thank you!

Fixes #20

Fixes #62

Ganbatte

Hi please correct checkstyle error !

Fixes #115

#91

Can consider switching to using TablePrinter to print help in standardized table format - look at code below for example. Columns in the table are separated by semicolons

Code:

Output:

Can consider changing to standardized input format, to remain consistent with the manual/auto tracker and finance tools input format.

Let's say we have CommandPacket packet = InputParser().parseInput(input)

The commandPacket will be as such:

Params can be accessed via packet.getParam("note"), for instance. This will return "skip page 70" as a String

Can consider refactoring code into separate function(s), to avoid "arrowhead" code (deeply nested if-else and loops)

See comment on lines +87 to +92 first before reading this comment: If goals in totalGoalList is changed to a hashMap then the goal for that month can be accessed with this.goals.get(month) in TotalGoalList.java instead of haping to use a for loop to search through all entries.

Try not to have unhandled exceptions unless necessary. Do add comments to explain the scenario(s) in which the exception would be triggered

Can consider changing the totalGoalList to a hashMap with key {month} and value {goal}, since we only allow them to set 1 goal for the month.

E.g.

totalGoalList

JAN: 1500

FEB: 2000

MAR: 4500

Consider renaming this variable - goalSetter gives the impression that it is a class that handles setting the goal, when it is simply the goal to be set. Maybe something like goalToSet would be clearer

Consider storing total monthly expenditure/income in a hashMap.

E.g.

expenditureList

JAN 1500

FEB 254

MAR 0

Increment/decrement the total amount whenever there is a change made. For instance, when a new expense of $25 is added to the ledger for June 15 2020, increment the total expenditure for June by $25. When this same expense is edited to become $45, increment June expenditure by $20. When this expense is deleted, decrement June expenditure by $45.

This way, there is no need to loop through all entries every time there is a new entry or every time you want to view the goal.

Avoid calling main() over and over again - this creates a new loop. So if the user does 10 operations, this will cause 10 nested main loops to be created. Should return to the top level main loop directly (i.e. use return;) and allow the main loop to continue

Good job

Could consider replacing -1 with a defined phrase so it is more reader friendly

@felixhalim say that we should include the space so that when user type "buys" or "buyer" it won't be detected

Not recommended to use * it doesn't fit the gradle checkstyles

Additional Spacing

Same reason as mentioned

do we need to remove spaces for this too?

I did this so it gives users the flexibility or options to type in "2" or "Day 2". so I dun think we should replace it?

General notes:

'convert /to'
'edit functions' (ForEx Rate, ForEx Abbreviation)

include foreign currency abbreviation

NullExceptionError is not fixed

will add new PR

Should we remove this branch since the loop will break at line 27 and update would not be called? Can probably replace it with an assert !isTerminate(command).

Should we copy into the new ArrayDeque before shuffling? Do we want to keep the original order of the Deck?

Idea for UG FAQ: What if my flashcard answer contains a single word "done" or "exit"?

Possible answer: Escape the command using \exit or "exit".

Sorry the earlier comment wasn't very clear. What I meant was that since we have


ArrayList<FlashCard> getDeck() {

    return deck;

}

then would passing the reference to the ArrayList to shuffle cause the original deck to be randomized also?

If we want to maintain the order in the original deck then perhaps a suggestion could be changing getDeck() in Deck class to


ArrayList<FlashCard> getDeck() {

    return new ArrayList<>(deck);

}

so that other methods cannot modify the ArrayList inside the Deck object except through add and delete.

Is there something missing in this diagram?

At section 4.1:

Would the diagram be more complete if there is a lifeline for the caller of ModTracker?

At section 4.5:

Should there be a ':' in front of the class name?

At section 4.7:

Should :TaskList return control to :Parser instead of User since it was called by :Parser? Also I thought the Parser is called by :Ui instead of directly by User?

Delete command for Normal Mode implemented by @alwaysnacy in #26.

Actually do we need JUnit tests for Help Commands?

Should we add throws Exception to the signature of executeCommand() in Main also?

I will create a new PR after fixing the issues.

Moving the logic to FlashCard class instead since that is where the response strings are generated.

Discovered bug in formatting when the flashcard id has more than 1 digit. Re-opening issue as a reminder to fix it.

Updated model component as well.

Looks good to merge. @kaijiel24 what do you think?

LGTM

That is a valid point, perhaps a target CAP of 5 is not the best example here.

However, the problem is reproducible with the following input:

Suppose I set a target CAP of 4.499999992, the following output is produced:

This is different from what is expected because getting a 'B+' for 4MC and 'A-' for the remaining MCs would allow the target CAP of 4.499999992 to be achieved.

On a side note, I expected the program to round any CAP close enough to 5 to 5.0 from the following behavior:

Would it be better if we used:

this.taskList.remove(index - 1);

We can also store this inside the taskList class as a method! What are you thoughts?

Just to note, we can replace this magic number!

We should try to remove these -1 as it's a little confusing without any context

Oh like what's the significance of -1? Or its supposed to perform like if (notebookIndex > 0) ?

LOL

Will update on my side once we have merged this first UG

YES thank you sorry was loading the JAR yesterday but I passed...

So it no longer removes pages based off titles as an input?

Oh okay thanks! Will make edits! I think something went wrong during the merge conflict, was working fine on my end just now

OMG LOL SORRY

Oh even if the date formatted is wrong? I thought it needs to be in the format dd-MM-yyyy or can it be the other way around?

Ohh okok will change it to just staying the notebook mode then

I can include that in for the test case

alrightoesss

Oh shoot okay, LEMME edit

Overlap with Long

Able to save the name of the user in the nameOfUser.txt file, however during the first iteration does not save the name immediately. Still currently working on it

Do y'all think it's necessary to include all the isPersonalised() and isTriggerAutoSave() methods?

Formatted the numbers

The IO test will fail now since there's a welcome message right? Or is there something wrong with my code HMM

OOOOOOOORight it was merge conflicts... lol

Ready for review

To edit the appendix F for corrupted/missing data files @yAOwzers

To edit UG: Delete the nameOfUser.txt file upon downloading, to include change of names instructions @yAOwzers

@chuckiex3 okay so we just need to reformat the help message right?

Should probably git ignore this? Or intended ya

I think it'll be clearer if you put


assert cardIndex <= flashcardDeck.size() : "card index inserted should be less than size of deck at this step"

assert is used to check if the value of something is what it should actually be, in this case, you want to assert that card index is actually less than or equal to deck size.

Consider adding another check for cardIndex to be greater than 0 btw! Otherwise negative values might break the application.

Just adding on, the more general alternative would be testAddCard() !

Remember to apply the renaming to delete card and write to file as well.

I think need to Figure 2 instead of figure 2

Same thing here

Are we keeping this line?

Everything else looks goood

I'll merge and help edit this since I'm working on it, thanks for pointing!

Assessing different features of the application

User should be able to enter and exit a feature of the application
If possible, the user can jump from one feature to another without going back to the main menu

LGTM, gonna merge and refactor any parts if necessary. Noice mate.

I agree with the check cap thing Sihui mentioned, but let's leave it to v2.0

LGTM, it won't affect other parts of the code atm since it only implements list grade function. Will merge this PR!

Will link the specific PR to this issue when working on it. Thanks for creating the issue!

Nice work Florence! Functions look good.

I resolved merge conflict between this branch and the latest TP master (after merging Shiho part). Then found out there are checkstyle errors. For the interest of time, I'll merge this PR in and fix the checkstyle on my side locally.

LGTM, failed some checks due to very little checkstyle error, will fix in the upcoming PR. Merging it!

Nvm just merge first! Thankss

LGTM, thanks for the prompt addition. Merging it 😸

LGTM, merging it!

Need to implement for the bookmark and academic features. Flashcard and timetable local storages have been built by @slightlyharp and @hailqueenflo .

Thanks for adding the log!

Remember to close this issue if it's done!

end result

LGTM!

Sorry wrongly approved, wait for new push before merging

This issue has been solved by each individual working on their feature

bookmark storage -> @lingsihui

academic storage -> @luziyi9898

timetable storage -> @slightlyharp

flashcard storage -> @hailqueenflo

Closing this issue since it has been implemented

Planning to standardize UI tmr

Adding on. This is cause the file is only really created when there's data inserted. Otherwise if left blank, it won't "create" properly.

@hailqueenflo added it

@hailqueenflo added

Closing this issue as it's implemented as "highlight" function

Merging this despite failing check. The check failed due to a JUnit test from @slightlyharp .

Merging it despite failing test for the same reason as @luziyi9898 PR

@slightlyharp Can you check your JUnit test again? It's still failing. Gonna hold on to your commit until noon, alright?

Fixed! Sorry I missed it, thanks for pointing it out!

Checkstyle error!!!!

I was thinking if it would be better to place quoteToPrint inside your toString() method and implement printAllQuotes() inside TextUi class instead? Then all you need is to call quoteList.toString() inside your printAllQuotes() method to print all quotes.

What do you think? 😃

Why do you create a DeleteCommand inside your todo package?

Likewise for this and other commands. Ideally all of our commands should be defined under the "commands" package

Not sure if this line of code will fail, but I think it will be better to use information.isEmpty() or information.equals("") ?

no worries I think that is not too big an issue

By right execute() should take in Storage as parameter and this line of code should be storage.save() instead due to the previous commit I pushed just now. Not sure why Git did not flag this as a merge conflict though.

Can you try to pull the latest one and see if the code updates?

Just to confirm, will your bookmark work if you do not 0-index this?

Oh I see how your code works now. As a suggestion, it will be better to 0-index the number here, so that you will not need to -1 from other places of your code such as your delete() and findByIndex().

I will assume that your code is working and approve it.

i think this could be previously from Yilun's one. Just now the log file got merge conflict so I just anyhow remove some lines haha.

Yup you're right this line can be removed 👍

I think this line should be in one of the methods instead of being declared outside

Maybe it would be clearer for the instance of ModuleList to be called modList instead? It might get confused with module.

Maybe can include a file creation here so the user need not manually create one

I think the address can be refactored into a constant e.g. FILE_PATH

Maybe can label as a comment what the various index of the array modInfo refers to

A suggestion is that this string can be set as a constant in the UI class and then you can call the constant (UI.constant name) here. If the message is changed at the other class for whatever reason then this part will automatically change also so don't really need change manually every time.

Not sure if really wanna use this as this is kind of buggy. Might need to update/ tweak cmd for it to show

Hmm, then for now we just use it. If it was encountered as a bug during testing then we change? Cos now this method is not tested yet right?

Sounds like a good idea!

Okay sure will change it back

yeah should ignore

Fixes #23

Looks good to merge!

The picture doesn't appear on the DG page

The sequence diagram is a bit messy and complex

No need these.

-1 later because range check is done in the later part.

-1 here.

Perhaps variable name change to index?

Fullstop before new line.

Expand out, should be highlighted in code style.

specifically line 37

Could be more specific in naming convention. For example, "getList" or "getActivityList"

Might have to SLAP this later on, once the base parser class Is up

You can use the methods in findDescriptionCommand class and assert using those methods.

Same as the previous comment

Nice slap!!!

Assertions.assertThrows() might help for exception testing

An example is given here:

https://howtodoinjava.com/junit5/expected-exception-example/

Spelling error for calorie

Looks good!

The variable commandOption is not used, perhaps we can use a if statement for now since there be more options later for the watchlist command.


if (commandOption.equals("-n")) {

    // create watchlist code

}

Do you still need this? Otherwise, it would be better to remove the old code.

Capitalize the first letter, same for line 314.

Is it intending to log some workspace information here after it is saved? There's a ":" at the end for line 119.

I think you could consider using a constant here.

Looks good!

Is there a reason for the chunk of space between the end of 4.4.1 and the header of 4.4.2?

Noted on that, I will add an extra line.

Noted on that, I have updated to use a constant for it.

Alright, I will add some comments to explain the numbers.

Okay thanks.

Ok, I have removed some redundant words in it.

That's true but because it affects the data folder in the src/test too, hence I made it specific. Perhaps, we could add it back after this PR as by then, the test files required for the JUnit would be uploaded already.

Yep, I think we could standardize to 2 periods, ".."?

I think we could use it during the initialization process? Seems like the only place it would fit.

You are welcome 😃

I think so because we are gonna have another subsection called design consideration, I guess having just current implementation should be fine!

@ChanJianHao Yep, we can consider adding in future versions, it's probably not a good idea to add in new features now. Perhaps, we could indicate in our User Guide that we will add support for remembering the last accessed workspace in future implementations.

@AY2021S1-CS2113T-F12-2/developers I have assigned everyone to this issue since this is a shared problem as well. I'm thinking we could state in our UG what is considered as valid input ("1") and invalid input ("1 ").

There should be a space after the # in order to display the heading properly (### 1.1 Viewing bookmarks in each category:)

Remember to add an extra line after add LINK for it to show as the next paragraph

Extra line needed here too

Should it be rm 1?

Should it be add LINK?

There should be a space after # so that the heading is displayed properly on GitHub

i.e. ### 3.1

Is this supposed to be a bullet point? If so, an extra space is needed after *

Perhaps you can also consider putting it in under Useful information: to improve readability

Close #125

Why not just take in Module as an argument?

Is "checkIfModuleExist" a better name?

Should we change -1 into a variable called INVALID_WORKLOAD or INVALID_WEEK too?

How about using COMMAND_EXIT.length() instead?

Should we gitignore this file?

Is it better to use the verb form "analyse" for the command instead?

The file is already created when the Storage object is initialised, this part is just a check when trying to read from the Storage object.

Okay!

LGTM

Noted on these changes.

😂

LGTM

LGTM!

Thanks for standardising.

Alright thanks!

Fixes #7 .

Fixes #8 .

The printing of the cheatsheet names should return a String. All the Sytem.out.println should be handled by only the UI class.

^same as first comment

The test is well made, but I think you can move the test into the Test package instead of putting it inside the cheatsheet list class

Nice Job! Very Neat and the OOP is damn legit

Legit!

Looks fine to me

Did you just list it one by one?

I think it is a little bit counter-intuitive because the user do not have any specific instruction in the UG or in the help page on how to add new tasks. Your add command makes it more confusing for the user to add the tasks.

I added another attribute in the cheatsheet, isFavorite. Also added a method setFavorite() in the cheatsheet class

I think you should not put this inside the constructor

Should we use break instead of return?

As long as it works I guess ...

Since the variables are already declared, why not pass use them? Like ShowDuration instead of Integer.parseInt(2)

What if we don't know showDuration?

Does this function prints out the review?

I just made it so that it prints out hours and minutes. Still cannot parse yet.

Ok, on it

Great job

This is done for add

add >Module Code> >1> >Number of Lectures>

Do you think the above works?

Is this done?

Done

What was the input you used, so I can replicate the problem

ok tried again and it works

Should be fixed above

May I suggest the use of backspace?

Has already been fixed in #142. Someone double check?

Good job!

Good use of tests! And good formatting overall

looks good

Changed existing interfaces into classes under 2 broad interfaces, making the code tidier. Looks great!

Looks great!

Thanks for resolving the anchors here!

Implemented

Try use commandLibrary as well?

Since it's the same we can refactor the two to a shared package then use it for all parts?

Printing response can put in Ui class. Response content can be a constant

Can try use/adapt the getInput method from Khoa's Ui package. That may standardise it

Can create a new instance of workoutSessionUi and make the method non static. That would be more oop and better design

Can try convert to throw Schwagserexceptions which can be handled by SchwasadsfgerHandler. That will help format the output which aligns with Ui output format

ohh tkss. Will fix that

Icic. Will check on that

Good refactoring to avoid circular dependency

good effort!

Nice refactoring

Nice work! Will follow the format

I think the reason why I was not able to exit your sub program is because your event loops here are slightly convoluted and possibly buggy. Please review!

You can try reviewing LocalDateTime methods for better ways to parse months. Likewise, my DateTimeManager class and parseRawDateTime method in InputParser class may be helpful to you in this particular function.

May want to refactor into another function

So you used toString() to handle save text output. You can perhaps introduce another method to the classes, possibly with an interface, to print some kind of save statement separately. This make make your life easier when you parse.

In case you cannot find a better way to deal with this, look at my previous comment

Noted. Will work on it.

Please take a screenshot, and report your input sequence.

Can you look into the following:

Your main loop do not have an exit condition. That's why you cant exit back to main menu
You are not using MenuPrinter in your feature. Hence MenuPrinter.prompt does not print error message for you.
May want to look into better presentation of error message
You may want to augment your command menu with what the user can input, I was not sure what to do

Please fix 1 and 2 then the PR will be merged. The rest LGTM.

Yo, please check out this issue: #47

Please review the changes and let me know if you feel something better can be done.

You will need to do some slight refactoring. Kindly do so then it will be good to merge.

The above top class design is too messy with arrow and it might confused the reader. A proper arrow head would improve readibility to the readers about your application.

The circle portion of the diagram I have marked may be hard for user to get a better picture of the flow of your design.

Perharps, use straight lines or adjust the different box accordingly so that user can follow through the flow of the design

The above top class design is too messy with arrow and it might confused the reader. A proper arrow head would improve readibility to the readers about your application.

The circle portion of the diagram I have marked may be hard for user to get a better picture of the flow of your design.

Perharps, use straight lines or adjust the different box accordingly so that user can follow through the flow of the design

The arrow coverges too much in the circled portion and thus it is making the class diagram more complicated than it should have been. Maybe try to simplify the diagram by breaking it into portions or structure the arrows in a way that it is straight and pointing the class table more clearly.

Inconsistent arrow used (dotted and solid arrow) and the arrow head in the circled area is over the box which should have only been hit the box only.

Lack of system architecture diagram to show the overall design of the program. (only uml and sequence diagram is available).

For this diagram, it is hard to interpret if it is a uml class diagram as the boxes used are wrong and format of the diagram is not consistent with the format of a standard uml class diagram.

The circle highlighted portion for the sequence design is very small and hard to see the different components/operations. It would improve the visusal better if more spaces are given to the main ravi box for the sequence diagram.

Hi, I have made the changes mentioned above and updated it. Kindly check if you can merge it. thank you!

To create different user acoounts :User Class- > name,password,7 Arraylist timetable (one for each day)

Inside every timetable arraylist put event class->name,location, timeStart,timeEnd

For each features:

Add feature->similar to add event command in IP

edit ->members of event class

display->sort timetable then similar to List command in Ip

delete->similar to delete command in Ip

clear-> delete for all events in timetable arraylist

compare->linear search thru timetable arraylist

command->program to prompt user for input individually (one at a time)

Mentioned in Enhance the add verifications of Input #65

No, I think break might cause the program to terminate as if a "bye" has been passed in. Not sure if it will break the outerloop in the main Duke class.

is it possible to throw more than 1 exception? Does it even catch the IndexOutofBounds?

well done with this part

Can I just check if our code can parse hours on top of minutes?

Ah okay, will raise an issue.

No it's not inside the constructor, I'll move it to the bottom so it's clearer haha

Just assume they know or set default.

Good catch, whoever is in charge of the add command just adjust accordingly.

Additionally, fixed some bugs related to adding show (would've caused null pointer exception)

Added test methods as example for how to write tests and specifically how to reference the ShowList

Also abstracted String related commands to StringOperations class

Nevermind fixed it...

Sorry don't merge yet, there's some bug with one of the commands

Just to explain the new changes for edit commands :

getRawEpisodesForSeasons() used instead of getEpisodesForSeasons() -> this one checks array using season-1 so it'll lead to bounds errors

If numSeason increases:

  - set a new array for episodes and init the new seasons with 1 episode

if numSeasons decreases:

  - replace episodes with smaller sized array and delete the episodes in the later seasons

  - initially episodes=[10,12] for 2 seasons, when resized to 1, episodes=[10]

Fixed issue.

Might want to try using Ui.print.... instead of just printing using System.out.println() in the individual functions.

Nice, may want to remove the empty test functions before the final release.

Good use of the Ui class!

For the date part, just raise the issue and I'll work on it later. thanks

Thanks for cleaning up.

Good cleanup.

Yea, I think we should include it in the UG so it'll be easier to use for the pitch/demo. Don't think it's necessary for the DG, maybe a quick note in the abstract or intro about the student part would be sufficient.

Good work with the review commands! Thanks Bryan.

Might want to append a "Programing Language: " cos the parser currently looks out for it bfore parsing the language

not sure what is this for

Might want to append a "Contents: " cos the parser currently looks out for it bfore parsing the language

Perhaps a a better word than pattern for the variable name?

Might wanna apply slap more here since iza little arrowheaded

Noted! Thanks for highlighting

Initially did that to cover up visual bug. Resolved!

Noted! Will rectify them.

Alright! Thanks for the suggestion.

Got notified by intelliJ to use it XD

Noted!

Okay

Line47 "AA1" should be "AA2"


        busesInZoneA.add(new Bus("AA2", AA2));


    KENTVALE("Kent Vale","Accommodation for visiting faculty guests"),


    UNIVERSITYTOWN("University Town","Place full of fun"),

the *

didnt we add these last time

Yes, currently it will show

might be a little weird because both AA1 and AA2 would show "PGP-> ...-> PGP" right now if that's the case, but I will take note of that for future updates

i'll try that

Can, but I was following the structure from ethan's skeleton code, the methods are all in FavList

This command is meant to be like a search filter (the way you would search for documents in your folder), so that users do not have to look around for correct faculty names. But this may get a bit confusing for a few users. @mrwsy1 what is your take on this?

Yea maybe I'll update the UG portion to say u don't have to key in the full name of the faculty

OK will add feature to show list of faculties

Will fix. Thanks

Will take this into consideration. Thanks

Noted, thanks

Café, will take note.

public String printPostQuizQuestion(int l, String correctnessLogo) {

return question + correctnessLogo + "\n\n"

Add one line here:

String correctnessLogo;

System.out.println(quizzes.get(quizIndexes.get(l)).printPostQuizQuestion(userAnswerManager.

                            getUserAnswers().get(l), correctnessLogo));

Resolves #129

Resolves #189

Prints time instead of date

Feature has been improved to be able to print events of next week in issue #280, resolved by pull request #281

Sam I think you got to make this private and make use of getters and setters, bcus i dont think it's good practice to make class members private.

Sean why ah ? We making all the attributes in the other classes private anyway right

Sean just one question over here ah, we will only able to accept one param over here issit? something like "select t/2". But will this block of code be able to accept a command like "deadline t/2 d/2020-10-18" where there are two params ?

Dude over here we passing a string for the priority ryt, You want to change it to an enumeration??

Riaz, just one doubt over here, for the priority are we using numbers, or are we gonna explicitly mention "High","Medium", Low", because in the user guide we inputted numbers for the priority.

yeah i'll do that once we finalize the project name, just kept it as duke exceptions because that's the intial name we used to create the class

Yes even i thought it would confusing because of the way the code is structured, but there's a reason i kept it like this, will explain it to you guys over a zoom session

ok sure but in which is the case that a null pointer exception will be thrown, prof said there is no reason we should catch it in this sectoin ryt right ?

Guys tried to make the code over here look more modular using SLAP as sean suggested and implemented the commands to adhere to the user guide. lmk what u guys think

yeah dude, as for now we are using strings for the priorities ryt and I'm assuming the priorities we'll be assigning to the respective tasks will be numbers since that was how it was mentioned in the user guide. So as for now I'm just sorting the tasks by comparing their priority strings and it will arrange every task in ascending order.

Only fats is plural here.

Consider making all related names plural (might be more suggestive of type) or all singular.

I disagree with changing this to a static method. Instantiate the FoodList object instead (or get a pointer to the object via argument parameters) to call this method.

Consider writing tests for the newly added DateTime functions.

Recommend splitting the class/avoiding a do-it-all class if you find this applicable.

The build tests are failing because of the dependency of the code on the Food class. Have you tested the code locally?

Possible workabouts:

Consider moving changes to a branch that targets snowbanana12345's Food class.
Otherwise, consider implementing a minimal Food class which we could also remove when merge conflicts arise later.

Changes to testing will be made in a new branch since this branch already has its core functionality. (removed fix #26)

Plan to further streamline these features by adding more FP support and reducing repetition.

I currently disagree with the proposed change in the above comment. I think that the Loader class itself should not be dependent on these other classes (Person, FoodList/Food, etc). This is to streamline dependencies. Current implementation of generic Loader looks good to me. I propose the use of class specific wrappers or child classes of Loader in future updates.

LGTM.

Resolved in PR #53 onwards

Partial fix deployed in PR #103

This fix introduced references to the FoodList object in methods that required an object instance of FoodList by accepting it as a method parameter. This might not be the intended/desirable way to setup calculator.

Calculator should be revised to either have a seperate class that interacts with FoodList, or have more ways of getting a pointer to an instance of FoodList.

Just a small adjustment, maybe can just change the start date to 28 instead and end on 25 as shown in your sample output.

Rest all LGTM

Ahh alright i see

This part, could you change it to String isReturnString = (isReturn) ? "1" : "0";

oops. alright thanks

Alright, will make the adjustment for all

what's that?

removed

I see, thanks for the edit!

Fixes #35

Looks good!

LGTM

Done!

for the failures in checks, go to yr project on IntelliJ and on the right side, you'll see a gradle tab, open it and then under the verification folder, run test and run check. it will show u the cases that has failed and then just edit them and re run the checks

LGTM! i'll merge this for ya

LGTM!

Will take a look at this and refactor my deleteCommand as well.

Sorry guys, accidentally created. suppose to merge into own repo.

LGTM!

yeah true, there will be a few more coming in soon. yeap sure 2.1

Thanks for pointing out! We will take note of this and make the necessary changes.

But there are actually modules with 0MC. eg. WR1401

Thanks for pointing out! We will take note of this and make the necessary changes.

Will most probably indicate in the UG that the due date will be 1 month from the loan date. As normally when u borrow a book , u can't really edit to your own preferred date. Most to most, an extension can be done.

Thanks for pointing out! We will take note of this and make the necessary changes.

Hi, this is still a work in progress, will most likely be ready in v2.1

Hi, actually if you do not input a priority, the default value of any task will be set as p:0. We will include this in the User guide so it will be clearer for users.

Thanks for pointing out! We will take note of this and make the necessary changes.

Perhaps can extract this part as an exception so that someone reading the code wouldn't get distracted by alternative paths taken when error conditions happen.

Probably need to add another line of code to remove the module stored in the moduleList in the access class as well, the allModules variable seems like an instance of the moduleList stored in the access.

Probably need to add another line of code to remove the chapter stored in the chapterList in the access class as well, the allChapter variable seems like an instance of the chapterList stored in the access.

Perhaps can extract this part as an exception so that someone reading the code wouldn't get distracted by alternative paths taken when error conditions happen.

Thank you, I have changed all the separated code into setIsModuleLevel().

Thank you, I have changed the constant to all Uppercase and changed the case to relevant constant

I have modified the method to return boolean instead of String because the execute method will need to print several messages for standard situation, therefore, I use boolean for convenience.

Sorry, it is a bit hard to achieve for this method, because this print statement is part of the while loop, return the message or throw exception will terminate the loop. Do you have any suggestions?

String array declared but not used.

Could be implemented below replacing in line 46: Instead of "AA1", could add Zone_A[0] and Zone_A[1] to show a sequential approach in preparation for larger number of bus routes. Keep in view for future commits.

Could refactor into a method adding all bus routes to the arraylist of bus object. For loop to run through and add the bus routes into the array list. This is to prepare for larger number of bus routes as we include other zones in our future versions. Keep in view for future commits.


        busesInZoneA.add(new Bus(ZONE_A[0], AA1));

        busesInZoneA.add(new Bus(ZONE_A[1], AA2));


        busesInZoneA.add(new Bus(ZONE_A[0], AA1));

        busesInZoneA.add(new Bus(ZONE_A[1], AA2));

Are you printing all buses and their specific routes together?

Initially, I added a some string functions to edit the getCommand(). However, in hindsight, the string manipulation should not be done in the ui function and I removed them. The UiTest in this case is trivial and not needed! I will close this pull request soon.

Edited it! Thank you for spotting that

Thank you! I will work on that in my next commit

Alright, will update the code accordingly to the most recent pull request by Yuxin.

Alright! Removed, sorry bout the oversight.

reafFile could throw a filenotfoundexception and that exception has to be dealt with.

We could refactor this into a method in the main function to make it look neater


# IDEA files

/.idea/

/out/

/*.iml



# Gradle build files

/.gradle/

/build/

src/main/resources/docs/



# MacOS custom attributes files created by Finder

.DS_Store

*.iml

bin/



/text-ui-test/ACTUAL.txt

text-ui-test/EXPECTED-UNIX.TXT

logger.log


logger.log.lck

*.log.*

logger.log.1

/logger.log.2

/logger.log.3

/logger.log.4

/logger.log.5

/logger.log.6

/logger.log.7

/logger.log.8

logger.log.1.lck

logger.log.2.lck


*.log.*

logger.log.1

/logger.log.2

/logger.log.3

/logger.log.4

/logger.log.5

/logger.log.6

/logger.log.7

/logger.log.8

logger.log.1.lck

logger.log.2.lck

Dine options also include BTC campus?

Alright will change all mentions of Kent Ridge Campus to NUS Campus


logger.log

logger.log.lck

*.log.*

logger.log.1

logger.log.2


/logger.log.2

/logger.log.3

/logger.log.4

/logger.log.5

/logger.log.6

/logger.log.7

/logger.log.8

logger.log.1.lck

logger.log.2.lck


data/FavList.txt

META-INF/MANIFEST.MF

src/main/java/META-INF/MANIFEST.MF


6. Your screen should show the start screen of Nav@NUS as seen in the figure below.<br>

Looks good to merge to me!

Looks good to merge!

LGTM

Issue addressed! #78

Good description.

Perhaps it's time for a new ASCII name art? 😃

nice catch

Yep, just following the ToC for addressbook since their user proifle and value prop is both under product scope and only product scope is in ToC

sounds good

Yes because the main README is outside the directory of the tp folder hence it can't be accessed via gh pages, i've tried.

apparently not

done

LGTM

Update: completed JUnit Testing for BinaryTree

Agreed. This is a non-issue.

Making the ArrayList static seems unnecessary. Maybe consider private ArrayList>Event> tests = new ArrayList>>();

This method can be an instance method. Being static seems unnecessary.

Maybe add spaces in between the arguments.

Nice catch!

Isn't the list manager initialized before the load command is called in main?

Ah okay, initialize is called again when an error has occurred during updating in the even the data was written halfway

That's a good suggestion. I have updated the code to reflect the changes. Now the printing of quotes will be done with the TextUI class!

Got it updated to the PR!

Tutorial testing

Looks good as a start!

Looks good!

LGTM!

LGTM

There was some conflicts with the quotesify logs. Resolved it and merged!

LGTM

LGTM!

LGTM! Thanks for adding the launch and shutdown 👍

LGTM

Add a constant FORMAT

Looks good to me, once merged I will add this into the parser

Is it possible to also add a EditTaskCommand format like ur EditModuleCommand

i dont think i can abstract this switch statement due to the fact that it returns different methods based on the command_word_****? Any advice?

Looks good to me, no issues.

Can merge

could u add, a AddCommand.FORMAT

a constant that outputs the correct format on how a addcommand will be read

looks good, thank you for solving the bug that affects me

looks good to me, once 1 or 2 more ppl look thru n approve i think can merge

i updated my textui last night also, with regards to the printing of the list

got it, fixed help command in my latest PR .

The edit command is fixed with wang qins latest PR

solved with PR df04fbc6f689c65c0dea794eeb336d41ebcd15f3, merged

Please add the diagrams into the DG and explain with reference to the diagrams

RESOLVED ALREADY THANK YOU

accidentally closed PR

Please fix failing tests before merging

PLS re-review thanks

Please consider using switch-case. Otherwise, the last one should be a single else-statement. (https://se-education.org/guides/conventions/java/basic.html#layout)

can you probably extract the method out? Quite a number of duplications.

Hi Manuel and Wei Siong, I think we should define a Timetable Class and use it to create ArrayList>Timetable>. Also, it fails CI test. Can you push some changes to debug to code that cause CI to fail? Thanks.

So that I can quit the application in a manner expected by the developer. (e.g. correctly save all my class timetables in a text file)

Feature implemented

Feature fulfilled.

PR did not pass CI, please review your code again

Reopened because the bug is still not resolved.

User stories realised.

Feature achieved.

Fixed via #109

Good and completed assertion statements.

Current commit LGTM.

OK.

Ok, I fixed them in v0.6

Ok, I added the case where the activity level is high. I did not do so cause the paper only give suggestions for 4 activity levels.

Ok, thanks for reminder.

The build tests are failing because of the dependency of the code on the Food class. Have you tested the code locally?

Possible workabouts:

Consider moving changes to a branch that targets snowbanana12345's Food class.

Otherwise, consider implementing a minimal Food class which we could also remove when merge conflicts arise later.

I copied and pasted the Food class into seedu.calculator package and added the JUnit test.

I think just use the try part below is enough. Because when it equals "", it also throws NumberFormatException.

The same when it is null

Sorry, I commented out for testing some exceptions. Forgot to recover it.

Overall is good. Could we separate testParser into 5 parts? The method seems a little bit long.

Although I follow the step to enable assertion, I don't know why assertion is still not enabled.

fixed

Prof and admin are roles of members. We will specify it in the UG.

i think the flow of the 1st addmod diagram is great and easy to understand

2nd addmod diagram is clean and easy to understand as well, as compared to diagrams in 4.5 & 4.6, which tend to be a little bit more cluttered

There will be another class that runs the code so that we can lessen the code in Project class

Agreed, we can remove the static keyword to allow each instances of the class to have their own variables. Just to better prepare for our v2.

Updated this issue.

Sure. I did a simple update on parser to facilitate this.

This is just a temporary change before @homingjun make a permanent change to parser.

ArrayList should be totally removed from parser.

good to merge

Good job, looks good for now. We can further break the individual command actions into classes and inherit from the parentCommand.

E.g. addMemberCommand inherit from MemberCommand

Yup sounds good, we should standardise. I also suggest we can have data package to hold all the data classes like Project, Sprint, Task.

simply to put all data classes in one package, but this is all in development, we can change when we integrate later

I don't understand what you mean by all classes in a file. Maybe we can discuss it during our meeting tonight.

Sprint object to store tasks allocated to sprint from project backlog, with start and end date.

Looks good. We can further standardise where we catch exceptions after we revamp our v1.0.

I have merged the try-catch block and removed the duplicate function from the classes.

Have you tried enabling the assertions? Link

I enabled it from settings, and also ran someone's test code which show the thing is enabled. But then like if I run "project /view" from the start it still exits with error even though got assert, then I can only fix it with try-catch block. Also if I replace the "no project" errors from try-catch to assertion then the error message prints but the task is still added. I'm not sure if this is normal behavior. Maybe assertions cannot replace try-catch?

The error was because there is no check whether there is a project before executing ViewProjectCommand. The error has already been fixed by @gmit22 in his latest PR.

Looks good. The bug seems to be fixed.

I have merged his PR, so you can do a pull on the repo and see if the error persist.

Apart from updating all the existing SprintCommands, I have changed the ProjectManager to use Hashtable to facilitate the deletion feature of projects in the future.

I have changed the command to allocate task from /assign to /allocate so that I can match the /deallocate command

Will update the user guide soon

Removing of sprint is deemed inefficient as it will produce gaps in the Sprint Manager. The user will be able to fulfil the same purpose by reallocating/removing the task and editing the Sprint goal if necessary.

I think for command, private will be the better access modifiers, as there will not be any subclass to AddWorkspaceCommand.

I think for command, private will be the better access modifiers, as there will not be any subclass to SearchCommand.

Yea I saw, but I was considering keeping my custom messages and logging.

I think I will take up your advice to keep things consistent.

Thanks for the feedback.

I understand the rationale with PARAMETER_NAME but I am consistent with the rest of our teammates,

I understand your concerns regarding the conflict of the naming convention of AnimeList class and the array list object in the watchlist. I will take up your suggestion and rectify it to "AnimeData" to represent Animes from our offline data.

Totally agree!

Will it be a bug if the user inputs find [EMPTY_STRING] ?

Good suggestion!

Sure, will add.

This part I'm not sure. Could you explain how are we going to invoke the methods in this class?

Sure!

Sounds good

Okay, will do

I'm not sure. What do others think?

Looks good to me.

Good job! How about we implement only one help function for v1.0 first?

The changes look fine.

Close #17

I feel like we should highlight the commands only.

For example, in this pic, I think can change Timetable, task, deadline to another mark-up style, like italic.

Other than that, it looks good.

Sorry this was from Francene's commit haha.

Agreed! Would make things more readable/understandable

nice use of assertion here!

Noted. We will work on this later.

Oh I didn't see this comment just now before I merge PR. I will change it later.

Add the function of listing by category
Update User Guide
Update complete help message for listing and deleting, and update EXPECTED.txt

This line may not be necessary since you return in the next line. But other than that looks good to me

results.add (FILE_WRITE_UNSUCCESFUL_MESSAGE);

Alright, will change that before merging

Thanks. Have changed it

Instantiated by the parser function?

Oh I added it bc otherwise, when I use the add command function, it asks for the user input for the content of the note. So when loading the files I dont want to ask for the user input, therefore check if a content file exists (aka are we loading the data) and accordingly see where to get the content from

It creates an error

Alright thanks will update

I tried but Attribute.COLOUR_TEXT().toString() isn't a constant. So it kept giving an error

Ah alright ok will fix that thanks!

Yup Ive added a check for it

Can I close this, since we are using the add parser to add it?

#41

Maybe we can separate the events and deadlines in toString of TaskList

I merge this

A possible implementation can be to let the user add the holiday period e.g. 2020-9-30 first, and the display function will only display tasks without lesson if the queried date falls between the holiday period.

Reminder: we can edit things only when it is necessary (e.g. tedious to delete and create a new one)

I think it should contain two module list, (or maps)

One module map is for storing the loaded module map from the NUSMODS

The other one is for storing the modules added by user.

done

I deleted the exception throw. it is handled inside the func already

Seems like there's a lot of changes made... Can we get a more detailed description?

For Command part, I only added the editModuleCommand

For ModuleManager part, I only added a function to change the module code of the target module.

For Parser part, I only added a regex to match for editModuleCommand, and a prepareEditModuleCommand() to deal with user inputs

In the end, I added some Junit tests since it is the only way to test.

PS: I edited your Module class and added a module description attribute

Looks quite good. If it passes the checks, then can merge.

simple changes, can merge

I found the edit -m CS1231 CS3224 the CS3224 cannot be recognised.

Just saw it. I will look into the issue tonight.

It seems the parser cannot parse the "del -m CS1231" command

Good flow! Perhaps storage should be accessible only by ItemList, since storage should only be changed when ItemList changes.

Would you like to shift this command outside the while loop? will the reminder command keep reminding for every other command we write?

I think its good. I like that you check for the date formats before running your code

There are no exceptions to implement for Goal

Excellent, I think the code runs very well. Most of the exceptions have been dealt with.

So far I thnk the code is good, I can see that there is a change from storing date time status structures to using entire clonable events instead. I think this will be helpful later down the line when we need to sort stuff as now to access the repeated events, they have a similar structure

Overall I think the code on the whole is good. I can see that it will filter out all events that fall on the day the program is run. I noticed that the remind command is in the while loop. I was wondering if the intention is to get the reminder to keep reminding the user everytime another command is run.

Overall I think the code looks good.

I think the overall code looks good. I will try on my end to see if I can assist with the storage formatting. I will also see if I can reduce the number of nested if statements on my end as well

hmm isn't that his own functions and not the old Duke functions?

It would be best if you could add that as well

Yea we will be moving the printing stuff the Ui.Printer class. For now, these are for testing

hmm, I think you're right

there should be a @params here i think

Not sure but I think need to include the 'done' command also for test case

wow excellent out of this world! thanks for the help!

thanks boss

Can u add the add review feature in UG/DG? not sure how it works. thanks

On it boss

Thanks bro

should i include examples for one input commands (help,list,bye)?

the help list is already very long so not sure if i should cut down the unnecessary examples

For episode and season command, can we include a catch to make sure correct number of episodes input?

For now, i can move the episode number to 100 even tho i set the show to have only 10 episodes (same for season)

Still many many many formatting issues but looks ok to me for the most part.

feel free to change whatever errors u notice god bless

Nice work! The issue/bug also happens for season too, could i trouble u to implement the same for season?

Nice!

I think its okay to leave the time left today to be negative if the user overshot his watch time. I included a line inside (Your watch time deficit will be highlighted below 😦 ) To let the user know why there is a negative number.

Just assume they know. Dont set default uh

Very solid!

On it

I added into the printing list already

Hi i think it would be better to merge the Activity class with the ActivityList class that i created, ie change the relevant Integer type in the activityList class to Activity class

I think the moduleCode should not be set as final for future extendibility, so in case if the user has already typed in the expected and actual time for a module, and after that the user realises the module code is wrong, the user can edit the module code if the module code is not final, but still retain the expected and actual time.

The default Object.equals() method takes in the object of Object type as the parameter and I keep this method signature to overwrite the default method.

Yes, the function name is renamed to checkIfModuleExist.

Closes #37

Closed in #43 deleteMod function

Closed in #43 addMod function

Closed in #43 deleteExp function

Closed in #43 addExp function

Agreed, but I think that it would be better after all the functions are up after the first iteration.

will do

LGTM!

Looks good!

lgtm!

Is the chinese because of some settings? 😃

Yeah, didn't notice it. Should have used information.equals("") since information is String. Thanks for spotting 😃

Not sure why this is commented out

Great suggestion. I pushed a new update to implement the change you suggested.

e a /n CS2113T Tutorial /d next fri /t 2pm

Hi john! thank you

I will like to clarify, for the above example, is there any library that i have to add to recognize "next Friday" or its just if else statements?

Look at Event.java in https://github.com/AY2021S1-CS2113T-F14-1/tp/pull/52/files it was implemented in the last PR.

Having multiple, shorter alternatives to the long command form is part of the Shorthand Command productivity & efficiency feature set and is considered intentional.

Closed, not a bug.

This is done to allow users to read the output instead of being teleported several pages down not knowing where they last were.

We will accept this bug and increase the speed to make it significantly faster.

Hi Yuxuan should line 25 be "String[] splitInput = word[1].split("d\");" instead of "d\\"? We only use one backslash symbol.

Fixes #123

Fixed #143

Fixes #269

Maybe can use constants to prevent magic numbers

Ok thanks for the feedback!

Using \r in JUnit test only works for Windows. To make test platform independent try using StringWriter and PrintWriter.

maybe can try using System.lineSeparator() for line breaks?

Nice use of lineSeparator for cross-platform compatibility

すばらしです

Appropriate use of assert, good job

Closes #114

Resolved

You can check this in the parse method to avoid surrounding your code with this large try catch.

Since goal extends event, goal command and goal class requires changes as well. In short a goal cant be repeated.

Do not merge yet. Once reviewed to delete old classes not used by repeat anymore.

I'll update Ui prints methods for this commands, from printing the divider line twice.

Could change the tick and crosses to "Y" and "N" or maybe simple print "done" and "not done". Resolve by updating toString method of events.

The issue here is when viewing a repeat of an event that is not repeated. The command tries to retrieve repeatlist and print it but repeatlist is null. Easily resolve to print "event does not repeat" when repeatlist is null.

Same as issue #207

Same as issue #213

To check whether listing repeat of events is properly functioning. For some reason appear to be getting a null repeatlist.

Can remove this commented out line

Hashmap has been updating.

Resolved with addition of academic planner.

Resolved with addition of CAP Calculator app.

Update test for addModuleToUser

Fixed for other inputs with similar situation as well

Completed

Example when i manually edit the files

Implemented with view by semester function in academic calendar

Implemented with set SU in CAP Calculator

Implemented with storage classes, which supporting saving and loading.

Testing coverage completed satisfactorily

Edit message to show target MC cannot be 0 or less

This can happen if your academic calendar has modules and grades and if those modules do not have a cumulative GPA of 5 in the first place.

For eg, if in the first semester I scored a GPA of 4.8, even if i score CAP 5 for the remaining 7 semesters, my CAP will never be 5.0.

Please advise on the academic plan you have entered prior to this CAP Calculator set target command,

Thanks @LiewWS

Checking of pre-requisites and co-requisites is a feature slated for the future. PlanNUS is still in its early stages and we will be increasing its functionality as the development cycle goes.

As stated in the user guide, users should just enter acadplan. Refer to the command summary of our user guide for more info.

Thank you for your kind suggestion. This will be taken into consideration for future iterations

This will be looked into in the future to see how we can best increase the data integrity of the database we currently have.

duplicated with #215

Folder containing logs will be implemented in future iterations

Update messages to show which parameter was wrong

duplicated with #218

duplicated with #186

Hi @LiewWS thank you very much for this report! We will look into it! Appreciate the follow up!

Can consider refactoring to reduce arrowhead coding

I removed it so that if it detects eg "buys" which contains "buy", it will print an error message and an example on how they can use the [buy] command, what do you all think?

Added in cases for error handling, thanks!

Good work. I think information as a parameter of this method is redundant as it can be accessed across the Class DeleteCommand though

I think the question mark is caused by my default language of the system (CHINESE), i will look for ways to fix that

thanks for the suggestion!

Perhaps can add "streamlines information for easy viewing and management"?

Since validSummaryMonth, summaryYear and summary throw no exception, you can move them outside the try block.

Fixes #18, #10, #8

Fixes #74 #73

As a user ready to start using the app, I can purge all current data, so that I can get rid of sample/experimental data I used for exploring the app.

#84

implement save load for Personal information , the class can be easily modified to include more fields

implemented method to convert data base output to String

Ya. Maybe we can have this command as "list tasks sorted" while keeping the original "list" command as unchanged?

Format category INDEX c/CATEGORY (e.g. category 1 c/academics)

Or maybe we can just have one big constructor ListCommand, i.e. ListCommand(int priority, String category, boolean isSorted) and pass these arguments in CommandCreator?

I decided that Lesson will be a sub class of Task. Lesson objects are special in the sense that they have a frequency. For now, I only considered the case where the Lessons are held weekly. Biweekly Lessons might be harder to implement, will think through again. Adding the Lesson class and enabling it to be added to the ScheduleManager and ModuleManager was the main bulk of what I tried to implement.

I still haven't changed the Deadline and Event classes to suit our TP code, which means as of now, Deadline and Event objects cannot be added to the ScheduleManager and ModuleManager yet. I also haven't added Junit tests and update the UserGuide.

At the bottom of ModuleManager, should put all sorts of task under it - Lesson, Deadlines and Events.

At the bottom of ScheduleManager, should just put in the Deadlines - should put down deadlines today and future as well. Will think of how to implement this and update later

@t170815518 probably will just remove this and stick to your add module method to add a module.

Should we enforce users to add a module to the module manager first before allowing them to add a task?

@t170815518 I'm just going to remove my add module feature but take a look at yours to see if there is this issue too

Will just remove this implementation

@t170815518 again i will remove this implementation but check if your implementation have the same issue

I removed this feature cause I realised when clashes are allowed into the timetable, the storage function will be quite weird too when loading the tasks when the app is starting

resolved by adding another line to inform user to enter a valid time

tested out again and didnt face this issue though...

changed already

added a new exception to handle invalid date format dates

added the bye feature at the bottom

settled the minor documentation error

dk what he talking about the 25 mins, but handled the error of an invalid LocalTime.

solved and also handled case of empty description for event and lesson as well

Handled the error by handling incorrect dates

updated user guide to make it clearer

Handled by removing this feature and just not allow user to add tasks when there are clashes

will need to make UG clearer that it will only be added in dates when there is school

tested out and seems like its already been handled though

Understandable but the reason for all is because all command is used so there would be like 11 import statements if i am specific? what do you think?

Add read courselist feature

Associate this issue with milestone 1

label : priority high + task

author : Rafael & Chu han

Description: recommend to split this into 2 different smaller issue as per UG. eg.

Ability to focus on todo
Ability to focus on deadline

please specify the issue.

note that if you added duplicated task, it will not be added to the list.

Here you go, i redownloaded it

1 2 is NOT referring to the task name itself. if you look at our UG, you would see that it is actually an indices. meaning, if u type list, and you see

eat food
deadline meeting (by: Dec 12 2020 19:40)

u can proceed to use this command to add the task 1 AND 2 into both module CS2113 and CS2113T

Fixes #14

test succeed, will close PR now.

test is done:)

Nicely done! Some methods are quite long though, maybe we can break it down further?

To state in UG that turning off program will switch off all appliances and explicitly mention it in exitmessage

To do check in LocationList as well.

KIV

change to "toilet does not have any appliances"

minor bug

Appliance was remained on when p_reset was called

mention in UG & exit message

"hundred" is a string

Update UG for better clarity

Change printout message to be more explicit

UG stated must be unique name, program works as intended

KIV

needs to resolve conflict . Otherwise, it can not be merged.

Going to merge it...

Splitted into smaller issues

please check with gradle again

checkstyleTest and checkstyleMain

We found a bug in Delete Action. delete action can not delete when focus action is also executed. No errors, however. Just nothing is changed

branch -v1.0 has issue. it can't run main(). shows errors traced back to showText() in cli

Will implement on-off reminder function!

Update #19

Added non-mode-specific printPrompt to be used in command execution

#48 done

Approval granted.

Here you go, i redownloaded it

Your UG is too hard to understand. Maybe you should make it clearly

Btw, i have ask prof. He ask me to do on fallback team already. So i cant create an issue on it.

Good Work ! I will continue from here to do the JUnitTest on removing location.

Thanks Leon, solved the issue !

Delete Pull Request

Thanks for the feedback. Most of the code is repeated one. I will place them in a private function in the class

Looks good !

nice done

needs to resolve conflict . Otherwise, it can not be merged.

My "Resolve conflicts" button is still greyed out...

this is done alr

No longer need a selection region. Use Focus instead. Closing this with my PR.

Back function is no longer needed. Just use focus. Closing this with my PR.

I intend to do it like:

edit [-type] [index] [field=newvalue] ...

for example:

edit -task 1 description=blahblahblah date=blabhblh

edit -mod 5 MC=4

Pre-requisites: the target item must be selected. If it is not selected, it will be automatically selected.

Related: Need to implement "unselect" and "untake". Planning to use base actions.

Also, the edit should be able to change tasks for a module:

edit -mod 5 updateTask

will set the task of module 5 to be tasks currently in the selected list.

If nothing is selected, this will clear the task for module 5.

Once Completed, the module can no longer be Taken or Untaken or Completed or Graded or Edited again, but it can still be Selected and be Found.

Once Completed, the module ceases to count in the current MC, but still counts in overall CAP calculation and overall MC.

Once completed, module should be immediately untaken.

Only taken modules can be completed

Hi.

It is expected behavior. Because you do not yet have tasks with indixes 1 and 2.

Try this:

todo myTask1

deadline myDeadline 1 /by 10 10 2020

list

you will be able to see the task list with indices.

Then you will be able to use “add” command to add tasks with indices to the modules.

Best Regards

Bozhao

Sent from Mail>https://go.microsoft.com/fwlink/?LinkId=550986> for Windows 10

From: Aldo Maximillan>mailto:notifications@github.com>

Sent: Friday, October 30, 2020 4:46 PM

To: AY2021S1-CS2113-T13-2/tp>mailto:tp@noreply.github.com>

Cc: Subscribed>mailto:subscribed@noreply.github.com>

Subject: [AY2021S1-CS2113-T13-2/tp] Unable to add to the list (#219)

[Screenshot 2020-10-30 at 4 14 18 PM]>https://user-images.githubusercontent.com/60168552/97678773-531e9e00-1acf-11eb-93f1-4ff576f28751.png>

Hi, I am trying to use your application, I followed how to use the add command, but I cannot use it

―

You are receiving this because you are subscribed to this thread.

Reply to this email directly, view it on GitHub>https://github.com/AY2021S1-CS2113-T13-2/tp/issues/219>, or unsubscribe>https://github.com/notifications/unsubscribe-auth/AGNEEMKOUGEPD7WZ3JZHKMLSNJ4P5ANCNFSM4TEWVYAA>.

>img alt="Screenshot 2020-10-30 at 4 14 18 PM" width="1440" src="https://user-images.githubusercontent.com/60168552/97678773-531e9e00-1acf-11eb-93f1-4ff576f28751.png">

Hi, I am trying to use your application, I followed how to use the add command, but I cannot use it

Should you need any further clarification please do not hesitate to keep commenting on this thread.

>img alt="Screenshot 2020-10-30 at 4 14 18 PM" width="1440" src="https://user-images.githubusercontent.com/60168552/97678773-531e9e00-1acf-11eb-93f1-4ff576f28751.png">

Hi, I am trying to use your application, I followed how to use the add command, but I cannot use it

In short, you need to create tasks before trying to add tasks to modules. The program works as expected in showing "NOT FOUND" when no specified task can be found, and showing "index out of range" when your specified index is indeed out of range. Those are expected error handling behaviors.

I used the exact same command as the example, still cannot add

Hi. Would you mid providing a screenshot?

Firstly, you must indeed have tasks before you can try to add tasks to modules, otherwise of course the program need to show an error message.

Try this:

todo testTask1

todo testTask2

todo testTask3

list

Then you should be able to see that you have some tasks in the task list.

then, you can add the specified tasks to specified modules:

add -task 1 2 -mod EE2026

Should you need further clarifications, please do not hesitate to respond to this thread. Thanks!

I think it is a little bit counter-intuitive because the user do not have any specific instruction in the UG or in the help page on how to add new tasks. Your add command makes it more confusing for the user to add the tasks.

Thank you for your valuable feedback. Please report this unintuitive documentation on CATcher as a Documentation Bug. We will improve the UG in the future based on your advice. (this shows that we have not written the UG truly from a user prospective, and we will consider adding a short "usage example" session before the feature list in the next version of UG) Thanks!

Here you go, i redownloaded it

This is working perfectly fine as expected... because for your newly downloaded jar there is ZERO TASK created, and the index 1 and 2 are SURELY out of range.

You can try reading my previous reply.

For your convenience I am going to paste it here:

''''

Firstly, you must indeed have tasks before you can try to add tasks to modules, otherwise of course the program need to show an error message.

Try this:

todo testTask1

todo testTask2

todo testTask3

list

Then you should be able to see that you have some tasks in the task list.

then, you can add the specified tasks to specified modules:

add -task 1 2 -mod EE2026

Should you need further clarifications, please do not hesitate to respond to this thread. Thanks!

'''

Here you go, i redownloaded it

In other words, this is a High Severity Documentation Bug (as Prof says), because our documentation is unclear. Please report this, thanks

1 2 is NOT referring to the task name itself. if you look at our UG, you would see that it is actually an indices. meaning, if u type list, and you see

eat food

deadline meeting (by: Dec 12 2020 19:40)

u can proceed to use this command to add the task 1 AND 2 into both module CS2113 and CS2113T

Yes you are right. that's why its showing "Index out of range" instaed of "[NOT FOUND]".

Your UG is too hard to understand. Maybe you should make it clearly

Yes indeed, our UG is not clear. Please report it as a High Severity Documentation Bug.

Btw, i have ask prof. He ask me to do on fallback team already. So i cant create an issue on it.

It is OK, we can improve the UG to make it clearer based on your valuable feedback in this issue. Thanks! Your comments really made us understand more from the user's perspective!

Oh this one the idea is to make a stack of questions in jumbled order - that's why first shuffle then make a stack out of it. So it's like pre-determined order, but to the user it will be "random". Implementation-wise, it's just popping off the topmost question from the "jumbled" stack. Idk if this answers your question?

^ sounds good! I think in checkAttempt() method excluding {Punc} probably already catches "", if not then we can just add that to the regex to ignore. Great idea!

Oh yea definitely. I wrote this before I finalised the runGameLoop. Thanks for the great catch!

^ Just checked, can confirm {Punct} regex includes "". Will add this to FAQ portion of UG!

I misunderstood this question! I have since fixed the issue. Thanks!

Ignore failed text tests here; I modified Main to test Deck Commands.

A more elaborate 'game mode' will be focused on in v2, although minimal functionality for viewing flash cards will be implemented in v1.

Discussed as a team and decided not to throw exceptions within individual Command classes; instead all exception handling will be encapsulated in Normal and Deck Parser.

Suspect tests failed due to "\n" in Windows

Push to v2.0.

Game Mode:

Q: "What if my flashcard answer contains a single word 'done' or 'exit'?"

A: Escape the command using \exit or "exit".

Edit: Credit @LiewWS

Push to v2.1

Thanks for your input! Anywhere section is for generic commands that have same effect regardless of user's location within application. help commands are unique and distinct across different modes, which explains the decision to place it within each mode, rather than within the "Anywhere" section.

https://ay2021s1-cs2113-t14-2.github.io/tp/UserGuide.html#guide-format

Done

Settled. Seperated exception handling and more detailed error message.

fixed

#14

#31

#48

Looks good to merge!

All checks passed, LGTM

Checks failed. Minor edits required.

completed with #16

LGTM.

close #35

update

close #36

Very good!

not sure whether commLib will be more convenient or not if we only have "diet", "workout", "profile" and "exit" commands which don't really execute many things. Maybe a simple switch case would be clearer?

Reviewed code, good job.

Fixed list command in DietSession.

Fixed add command in DietSession.

Fixed delete command in DietSession.

Fixed list command in DietManager.

Fixed create command in DietManager.

Fixed exit command in main

Fixed exit command in DietSession.

Fix start workout session.

Fix end workout session

Fix remove workout session

Fix save diet session

Fix add workout moves

Fix list workout session

Fix delete dietSession

Add javadoc and loggings for diet

update help for diet

add diet storage and main menu in DG

hahahah

The logger has been updated for all the classes that I could find it in. Thanks for pointing it out!

Done! Thanks for the heads-up! 👍

good job

Just to explain the new changes for edit commands :

getRawEpisodesForSeasons() used instead of getEpisodesForSeasons() -> this one checks array using season-1 so it'll lead to bounds errors

If numSeason increases:

set a new array for episodes and init the new seasons with 1 episode

if numSeasons decreases:

replace episodes with smaller sized array and delete the episodes in the later seasons

initially episodes=[10,12] for 2 seasons, when resized to 1, episodes=[10]

Understand! But we need to ensure that users give a valid input. e.g. they change the number of seasons to 0. If we assume the user input is perfect, then it will work well.

a search function has been implemented

done already

I think its okay to leave the time left today to be negative if the user overshot his watch time. I included a line inside (Your watch time deficit will be highlighted below 😦 ) To let the user know why there is a negative number.

emmmmm, that makes sense. Then we just leave it. Thanks, Benardo!

Good to merge.

Looks good.

you mean the Storage storage?

Fixes #31

Fixes #32

Fixes #51

Fixes #52

Fixes #43

Fixes #42

Save edited chapter name and module name

Combine GoChapterCommand and GoModuleCommand

Combine BackChapterCommand and BackModuleCommand

Fixes #74

looks good

looks good to merge

looks good

Thank you for pointing out!

Got it, thanks for pointing out Wira!

I was not able to access them in the test files where I was trying to reference them. Changing from private to protected solved that problem.

Oops, my bad! I'll use the pow function instead

Sure, that sounds good.

Got it, will rename accordingly!

Right, I'll figure that out. Thanks Wira 😃

Agreed, will change.

Oh, but aren't we testing the getter functions inside the templates and not the components?

Okay good point - I didn't consider that. I modified the declaration as per IntelliJ's suggestion because if I remember correctly, it flagged a warning (but not an error).

But like @hughjazzman pointed out, they will still work when we declare them as final right?

Right, will remove and push again. Thanks for pointing out!

Sure, will remove it.

IntelliJ suggested it to me and I tried downloading it to experiment around, but I don't think it does anything particularly useful. I'll remove it now.

Sure, will change it to SPACES.

Sure, will remove it.

My bad 😦

Must have been by accident, I'll remove it now

Okay

Okay but it is already SetBooleanCommand.png right?

Sure, will do.

Ah, sorry for the confusion. I changed it to SetBooleanCommand.png already but didn't push it yet.

Okay sure

@hughjazzman Got it, thanks for the help!

Ok that makes sense

The code looks good to merge.

There's some conflicting files that needs to be fixed.

Looks good to merge

Looks good to merge.

The most fail-safe method is to create the map with the actual index of each module inside the full module list after loading. This causes an efficiency of o(n) but allows for the most robust map creation as the index in the map will always correspond to the index in the array list.

Thank you for the suggestion, we will look into this feature.

Thank you for the suggestion @Jingming517. We designed PlanNUS as a platform that provides its user with all the information needed to make the best decision. Due to this, we decided not to implement a limit for users when it comes to suggesting which modules they should SU as more information is often better in this situation. Nonetheless, we appreciate the suggestion and will look into it for future iterations of PlanNUS.

jar file link has been amended

LGTM

oh yeah I forgot to do this function

will do

good call

great idea

@zsk612 @CFZeon

will do

Good idea

will do

Great catch

My calls are all static tho, so printer has to be static

Oh I didnt see that, thanks

great catch!

damn I missed alot of stuff lol

do a gradle build test on ur own system before you do pr

Looks good

Lgtm

@neojiaern can be used in conjunction with my computeNewDeadline functions for revision implementation

@neojiaern simplified comparing of date to: if dueBy is before or equals to todays date. Function can be used to reference for revision in card version.

I cannot, because ui.showToUser enforces newline after printing string. I will use format string in the next iteration.

Dropped functionality

Merged

Failed text-ui-test, fix later merges

Need to explain "current time" and "lesson now" indicators in UG

Could you take a look at this xing rong?

UG for planner mode

Either add a warning in UG or change input code format?

Add warning in UG to follow the format.

Already working as intended of moving code to the top of screen. User prob has a large monitor screen so not enough text input to completely clear screen. ZH opinions?

I dont think screenshots are possible for these

trim?

Add warning in UG to read the command format before the features.

show monday views monday as a module declaration not a day declaration hence the error.

help is not a valid module in NUS so it errors.

Add warning in UG about appropriate MODULE names. BOLD and HIGHLIGHT so users can see

Add to UG limitations to the app?

same as issue #167

same as #159

Index error

same as another issue

Change wording of Quick Start to "You should see something similar to this Zoomaster logo indicating that you have successfully started your Zoomaster program."

Do you guys want to sort?

As per UG "The chaining of commands only performs on one module which is {MODULE}."

Add bold comment around ONE MODULE to highlight to users

same error

make UG more reader friendly

Naming error resulting in failing Gradle checks

UI updated by other teammates

Should I remove the previously created LoadComponentTest class and split it into the respective component test subclasses?

Yes, that's a good idea

Noted, thanks!

Yeah it looked redundant to me as well, will make the edits after we append the code.

Noted!

Updated it to cover the current implementation

I'm not sure about this bit. Maybe we could remove it later.

My point here is that if we did choose a linked list structure then we would have no templates so we just add the components directly to the linked list. Of course, we could block users from adding certain components by checking the components present in the linked list. For some reason, I feel like a linked list interpretation would have worked. Any specific reason why it wouldn't be able to simulate a circuit system?

Please let me know how if the way I've done LoadComponentTest so far makes sense, if it does I will extend it to cover all components

LGTM!

edit: After merge conflicts have been resolved

LBGTM! (Looks blissfully good to merge)

LGTM!

Need a review on the latest version

Update

@sevenseasofbri to do CalculateBooleanCommandTest

@oasisbeatle to do AddBooleanCommandTest

@hughjazzman to do SetBooleanCommandTest

I declare outside because it is used in more than one methods.

I think it is better to implement exception handling inside the respective methods. But I believe this method works for now to prevent the app from crashing first due to some weird inputs.

Is there a better alternative? I used the above method as using "\u2713" and "\u2718" violates the Gradle style check.

Yeah, can.

List week number, modules, expected workload and actual workload in a table.

Change the welcome screen to show a graphic of ModTracker.

Looks good to merge.

The word "valid" is defined in the preceding "notes" sub-section under the "features" section.

Currently 3 types of sorting implemented which are:

Sort by location
Sort by time
Sort by description

Fixes #51

I think you may also consider saving and loading user configuration as txt file in your storage class.

Already implemented

Noted

Completed @jialerk

Approved the merging of this pull request after looking through the code.

Successfully implemented the required features by @sugandha929

This is actually not a bug . The level of indentation is intentional but I think the user guide was not clear about what it means. I will update the UG.

perhaps be a bit more broader and specify potential causes of why Zoommaster was not able to add the slot to the timetable? I think I will do this instead.

Another way to address this problem is to use keywords in the command is to use keywords but

There is a simple fix to this. The previous method used split(" ") to split the words by one space. Instead I should have used split("\\s+"). This will combine all white spaces as a delimiter.

i think can use an image or emoji on the UG that attracts the readers attention so that they read the constraints.

It will not run for every command. I would prefer not to switch it out because there's a try catch

Your dateException is failing the test. Maybe try to fix the expected output?

Excellent!

nicely done! can consider adding in comments next time 😃

Well done! 😃

looks good

Looks ok.

I'll change the expected.txt after deciding the format of timeline.

Oh yes! Thanks for that reminder, I've changed it accordingly!

But isn't Ui being passed into the execute method of all Command classes?

Closed by pull request #118

syntax inferred:

edit index

the new event is entered with the same syntax as adding a new event.e.g. 'assignment xxx /t 2020-10-01 20:00 /l somewhere

in a later version, we shall allow the user to edit only one part of the event. i.e. only time or location or type or description

make sure you can pass CI. There is no bug in CI now.

coding style check failed

solved

Improved

the format is invalid in this case because 53:40 is an invalid time.

But nonetheless, I have updated the error message in this case.

Similar to the other issue raised by this guy. Improved in the same way.

solved

closing duplicate issue

Ok!

Achieved in Branch hr ver0.1

JUnit

close # 22

so that I will not forget or miss the event.

so that I can manage the schedule

so that I prevent myself from doing repetitive things

so that I can get information on events quickly

So that i will be reminded of the number of dats left

Fixed it!

Already implemented in PR to fix #103

Fixed by #120

Fixed by pull request #210

Re-open issue when applicable for v2.1

Fixes #69

hello

looks

overall well done just that there is minor coding style errors

i feel very attacked 😦

dk why the commit for addSet not showing here but its here: https://github.com/AY2021S1-CS2113T-T09-4/tp/commit/6d3bcb6d912b28042a904fd78c4f5b31fbfcfbdf

since the xxx.execute() method throws an exception, summaryYear.excecute() should be in the try block to catch exception if there is any

Looks good to merge.

Fix printEventsCommand and checkstyles Closes#36

Closes #38

already fixed. please ignore this issue.

Same issue, already fixed in PR #233.

Only available modules in NUS are allowed to be added! We will make it more clear in the UG. Thanks!

Thanks for pointing out! The same issue appeared and already fixed in PR #233.

Thanks for pointing out. Already fixed in PR #231

Just a suggestion maybe, add an indicator to show that there is additional information available.

Fixed in PR #231

Thanks for pointing out! Already fixed in PR #233

Thanks for pointing out! Already fixed in PR #231

added logo pictures and cover page for both user guide and developer guide

closes #48

closes #28

Added user guide details

ok I removed the file.

Is the scrumptious package name ok though

ok. I merge? Or someone merge after checking or sth?

good

sounds ok imo

Labels added and getTask() has been implemented for all applicable functions

This PR is deprecated. Task function will use Gunit's implementation.

Looks ok. I can just copy paste my part in after merging

Looks good

Looks good.

I have merged the try-catch block and removed the duplicate function from the classes.

Have you tried enabling the assertions? Link

I enabled it from settings, and also ran someone's test code which show the thing is enabled. But then like if I run "project /view" from the start it still exits with error even though got assert, then I can only fix it with try-catch block. Also if I replace the "no project" errors from try-catch to assertion then the error message prints but the task is still added. I'm not sure if this is normal behavior. Maybe assertions cannot replace try-catch?

I have merged the try-catch block and removed the duplicate function from the classes.

Have you tried enabling the assertions? Link

I enabled it from settings, and also ran someone's test code which show the thing is enabled. But then like if I run "project /view" from the start it still exits with error even though got assert, then I can only fix it with try-catch block. Also if I replace the "no project" errors from try-catch to assertion then the error message prints but the task is still added. I'm not sure if this is normal behavior. Maybe assertions cannot replace try-catch?

The error was because there is no check whether there is a project before executing ViewProjectCommand. The error has already been fixed by @gmit22 in his latest PR.

Just checked again, the code is working as expected now.

You are talking about the Unit Test assertions is it? Those assertions are different from the normal assertions. Normal assertions are placed in the code using the assert keyword, therefore the link I have provided only works for normal assertions. Anyway both have different purpose, so yeah.

I think the problem is I did not pull Gunit's latest code. Now everything looks good.

I made more changes regarding some commands without dashes, so maybe can review one more time before merge.

The basic functionalities are essentially complete(+added removal of corresponding retrospective if a sprint is deleted)

The operations for saving and parsing the sprint retrospective classes are missing in Project class. Without them, the data will always be lost after the program exits.

Thanks for the suggestion.

I have tried to implement the saving of the list into the project class, but now it will cause nullpointerexception when launching scrumptious with the presence of data.json. Any idea what issue this might be? I feel that my tojson and fromjson is very similar if not identical to the ones used in the sprint classes.

Edit: Gradle fail due to failed test classes as a result of nullpointerexception

sub task of #19

Closes #18

lgtm

Closes #85

Closes #152

Added in Colour and Checkstyle changes pull request

huh but then i think its obvious that the lesson now surrounds the second session 1600 to 1800?

heheh this is just printing the clear ecscape sequence, nothing i can do about it heheh. I think it serves its intended purpose of clearing the visible screen for the user though im not sure how his input bar shifted to the right

Looks good!

thank you

tP comments dashboard

1. DANIEL LIM WEE SOONG @daniellimws (195 comments)

2. WIRA AZMOON AHMAD @hughjazzman (164 comments)

3. CHONG JUN XIANG @Chongjx (163 comments)

4. R RAMANA @R-Ramana (130 comments)

5. KINGSTON KUAN JUN XIANG @kstonekuan (84 comments)

6. VISHRUTI RANJAN @sevenseasofbri (61 comments)

CheckList

7. NEO JIA ERN @neojiaern (55 comments)

8. WAMIKA MALIK @wamikamalik (47 comments)

9. TANG ZHI YOU @Zhi-You (46 comments)

10. SEAN TAN RUI AN @thatseant (43 comments)

11. DANZEL ONG JING HERN @dojh111 (40 comments)

12. SIM JUN YOU @f0fz (36 comments)

13. NGUYEN TIEN KHOA @tienkhoa16 (35 comments)

14. NG SHU LIN JANE @Jane-Ng (33 comments)

15. HENG FU YUEN @HengFuYuen (32 comments)

16. JERICHO CHUA WEI QUAN @jerichochua (32 comments)

17. CHAN JIAN HAO @ChanJianHao (30 comments)

18. MOOI SUET YENG, FRANCENE @chuckiex3 (29 comments)

19. CHEONG RAY YANG, MARCUS @Kafcis (28 comments)

20. DANIEL LIM ZI-JIE @daniellimzj (28 comments)

21. NEIL BANERJEE @neilbaner (26 comments)

22. Yeow Wei Liang, Brandon @brandonywl (25 comments)

23. SAMUEL LEOW WEI HAN @samuellleow (25 comments)

24. SEAN LIM ZHI XIANG @Aseanseen (24 comments)

25. VISHESH P ARORA @wish2023 (24 comments)

26. CHEN JIA JUN @iamchenjiajun (23 comments)

27. FELIX HALIM @felixhalim (23 comments)

28. LIM AN QI @anqi20 (23 comments)

29. TOLENTINO ABNER JR MORALES @abnermtj (20 comments)

30. CHUA SIN ENG, AMANDA @amanda-chua (19 comments)

31. JOSEPH ABRAHAM @josephhhhhhhhh (17 comments)

32. WONG ZHEN WEI @keke101 (16 comments)

33. ALSTON THAM YONG HAO @alstontham (16 comments)

34. PUAH SIEW WEN @e0425705 (16 comments)

35. HUBERTUS ADHY PRATAMA SETIAWAN @adhy-p (15 comments)

36. IVANDER JONATHAN MARELLA WASKITO @ivanderjmw (15 comments)

37. WONG HENG CHIN @EthanWong2212 (14 comments)

38. RIAZ AHAMED @riazaham (13 comments)

39. ONG CHIN HANG @CookieHoodie (13 comments)

40. WU YUXIN @Lezn0 (13 comments)

41. NAZRYL BIN KHAIRIL IDHAM LIM @Nazryl (12 comments)

42. EYO WEI CHIN @EyoWeiChin (12 comments)

43. GUNIT MITTAL @gmit22 (11 comments)

44. SAMUEL PAUL CHRISTOPHER @samuelchristopher (10 comments)

45. LUSI WU QI @Lusi711 (10 comments)

46. LING SI HUI, SHIHO @lingsihui (10 comments)

47. DUAN YU HANG @Artemis-Hunt (9 comments)

48. TAN ZHENG CHONG, SHAWN @ShawnTanzc (9 comments)

49. LIEW WEI SIEW @LiewWS (8 comments)

50. NEO YAO JIE, JOEL @yAOwzers (8 comments)

51. LIM YUAN BING @farice9 (8 comments)

Assessing different features of the application

52. TAY CHIN HENG @dozenmatter (8 comments)

53. LIM GANG LE @limgl1998 (8 comments)

54. MANUNTAG MANUEL II PAPA @manuelmanuntag96 (7 comments)

55. CHEN QIXING @QX-CHEN (7 comments)

56. RANI KARTHIGEYAN RAJENDRAKUMAR @1-Karthigeyan-1 (7 comments)

57. ONG DE ZHI @OngDeZhi (7 comments)

58. LIM SI QIAO FLORENCE @hailqueenflo (7 comments)

59. KER CHIN TIAN @kerct (6 comments)

60. LU ZIYI @luziyi9898 (6 comments)

61. ALDO MAXIMILLIAN SUGITO @domaxi (6 comments)

62. ZHOU SHIKAI @shikai-zhou (6 comments)

63. JOEL NG YI XIAN @joelngyx (6 comments)

64. WANG ZESONG @wgzesg (6 comments)

65. LOOI KAI WEN @kaiwen98 (5 comments)

66. NG WEI SIONG @weisiong24 (5 comments)

67. JAZHTEN LOW YONGSHEN @jazhten (5 comments)

68. THEODORE PINTO @theopin (5 comments)

69. WANG SHUYI @mrwsy1 (5 comments)

70. CHAN XU HUI @durianpancakes (5 comments)

71. SHREYAS KUMAR @shreytheshreyas (5 comments)

72. ELIJAH NG SHI YI @mxksowie (4 comments)

73. MUHAMMAD HOZEFA ZUMKHAWALA @MuhammadHoze (4 comments)

74. ZHANG JIAYI @gua-guargia (4 comments)

75. YEE ZONG SHEN JOHNSON @Johnson-Yee (4 comments)

76. DORIAN ALEXANDER FONG @dorianfong98 (3 comments)

77. ANDRE WONG ZHI HUA @AndreWongZH (3 comments)

1. DANIEL LIM WEE SOONG `@daniellimws` (195 comments)

2. WIRA AZMOON AHMAD `@hughjazzman` (164 comments)

3. CHONG JUN XIANG `@Chongjx` (163 comments)

4. R RAMANA `@R-Ramana` (130 comments)

5. KINGSTON KUAN JUN XIANG `@kstonekuan` (84 comments)

6. VISHRUTI RANJAN `@sevenseasofbri` (61 comments)

7. NEO JIA ERN `@neojiaern` (55 comments)

8. WAMIKA MALIK `@wamikamalik` (47 comments)

9. TANG ZHI YOU `@Zhi-You` (46 comments)

10. SEAN TAN RUI AN `@thatseant` (43 comments)

11. DANZEL ONG JING HERN `@dojh111` (40 comments)

12. SIM JUN YOU `@f0fz` (36 comments)

13. NGUYEN TIEN KHOA `@tienkhoa16` (35 comments)

14. NG SHU LIN JANE `@Jane-Ng` (33 comments)

15. HENG FU YUEN `@HengFuYuen` (32 comments)

16. JERICHO CHUA WEI QUAN `@jerichochua` (32 comments)

17. CHAN JIAN HAO `@ChanJianHao` (30 comments)

18. MOOI SUET YENG, FRANCENE `@chuckiex3` (29 comments)

19. CHEONG RAY YANG, MARCUS `@Kafcis` (28 comments)

20. DANIEL LIM ZI-JIE `@daniellimzj` (28 comments)

21. NEIL BANERJEE `@neilbaner` (26 comments)

22. Yeow Wei Liang, Brandon `@brandonywl` (25 comments)

23. SAMUEL LEOW WEI HAN `@samuellleow` (25 comments)

24. SEAN LIM ZHI XIANG `@Aseanseen` (24 comments)

25. VISHESH P ARORA `@wish2023` (24 comments)

26. CHEN JIA JUN `@iamchenjiajun` (23 comments)

27. FELIX HALIM `@felixhalim` (23 comments)

28. LIM AN QI `@anqi20` (23 comments)

29. TOLENTINO ABNER JR MORALES `@abnermtj` (20 comments)

30. CHUA SIN ENG, AMANDA `@amanda-chua` (19 comments)

31. JOSEPH ABRAHAM `@josephhhhhhhhh` (17 comments)

32. WONG ZHEN WEI `@keke101` (16 comments)

33. ALSTON THAM YONG HAO `@alstontham` (16 comments)

34. PUAH SIEW WEN `@e0425705` (16 comments)

35. HUBERTUS ADHY PRATAMA SETIAWAN `@adhy-p` (15 comments)

36. IVANDER JONATHAN MARELLA WASKITO `@ivanderjmw` (15 comments)

37. WONG HENG CHIN `@EthanWong2212` (14 comments)

38. RIAZ AHAMED `@riazaham` (13 comments)

39. ONG CHIN HANG `@CookieHoodie` (13 comments)

40. WU YUXIN `@Lezn0` (13 comments)

41. NAZRYL BIN KHAIRIL IDHAM LIM `@Nazryl` (12 comments)

42. EYO WEI CHIN `@EyoWeiChin` (12 comments)

43. GUNIT MITTAL `@gmit22` (11 comments)

44. SAMUEL PAUL CHRISTOPHER `@samuelchristopher` (10 comments)

45. LUSI WU QI `@Lusi711` (10 comments)

46. LING SI HUI, SHIHO `@lingsihui` (10 comments)

47. DUAN YU HANG `@Artemis-Hunt` (9 comments)

48. TAN ZHENG CHONG, SHAWN `@ShawnTanzc` (9 comments)

49. LIEW WEI SIEW `@LiewWS` (8 comments)

50. NEO YAO JIE, JOEL `@yAOwzers` (8 comments)

51. LIM YUAN BING `@farice9` (8 comments)

52. TAY CHIN HENG `@dozenmatter` (8 comments)

53. LIM GANG LE `@limgl1998` (8 comments)

54. MANUNTAG MANUEL II PAPA `@manuelmanuntag96` (7 comments)

55. CHEN QIXING `@QX-CHEN` (7 comments)

56. RANI KARTHIGEYAN RAJENDRAKUMAR `@1-Karthigeyan-1` (7 comments)

57. ONG DE ZHI `@OngDeZhi` (7 comments)

58. LIM SI QIAO FLORENCE `@hailqueenflo` (7 comments)

59. KER CHIN TIAN `@kerct` (6 comments)

60. LU ZIYI `@luziyi9898` (6 comments)

61. ALDO MAXIMILLIAN SUGITO `@domaxi` (6 comments)

62. ZHOU SHIKAI `@shikai-zhou` (6 comments)

63. JOEL NG YI XIAN `@joelngyx` (6 comments)

64. WANG ZESONG `@wgzesg` (6 comments)

65. LOOI KAI WEN `@kaiwen98` (5 comments)

66. NG WEI SIONG `@weisiong24` (5 comments)

67. JAZHTEN LOW YONGSHEN `@jazhten` (5 comments)

68. THEODORE PINTO `@theopin` (5 comments)

69. WANG SHUYI `@mrwsy1` (5 comments)

70. CHAN XU HUI `@durianpancakes` (5 comments)

71. SHREYAS KUMAR `@shreytheshreyas` (5 comments)

72. ELIJAH NG SHI YI `@mxksowie` (4 comments)

73. MUHAMMAD HOZEFA ZUMKHAWALA `@MuhammadHoze` (4 comments)

74. ZHANG JIAYI `@gua-guargia` (4 comments)

75. YEE ZONG SHEN JOHNSON `@Johnson-Yee` (4 comments)

76. DORIAN ALEXANDER FONG `@dorianfong98` (3 comments)

77. ANDRE WONG ZHI HUA `@AndreWongZH` (3 comments)

78. HO CHONG HAN NATHANIEL `@nat-ho` (3 comments)

79. TAN JIAN HUI TOBIAS `@tobiasceg` (3 comments)

80. YEAP CHUN LIK `@yeapcl` (2 comments)